firmware: dmi: Respect buffer size in get_modalias

When we collect data from DMI data the result is accumulated either in a page buffer from sysfs entry or from uevent environment buffer. Both are big enough (4K and 2K) and it is hard to imagine that we overflow 4K page with the data, still the situation is different for uevent buffer where buffer may be already filled with data and we possibly may overflow it.

Thus lets respect buffer size given by a caller and never write data unconditionally.

CC Jean Delvare <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

When we collect data from DMI data the result is accumulated either in a page buffer from sysfs entry or from uevent environment buffer. Both are big enough (4K and 2K) and it is hard to imagine that we overflow 4K page with the data, still the situation is different for uevent buffer where buffer may be already filled with data and we possibly may overflow it.

Thus lets respect buffer size given by a caller and never write data unconditionally.

CC Jean Delvare <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---
 drivers/firmware/dmi-id.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: linux-tip.git/drivers/firmware/dmi-id.c
===================================================================

Hi Cyrill,

Quoted 6 lines · Cyrill Gorcunov

When we collect data from DMI data the result is accumulated either in a page buffer from sysfs entry or from uevent environment buffer. Both are big enough (4K and 2K) and it is hard to imagine that we overflow 4K page with the data, still the situation is different for uevent buffer where buffer may be already filled with data and we possibly may overflow it.

Would it not be a concern if that ever happened?

Quoted 2 lines

Thus lets respect buffer size given by a caller and never write data unconditionally.

On the principle I agree. On the implementation, not quite so.

drivers/firmware/dmi-id.c static ssize_t get_modalias(char *buffer · line 108

Quoted 11 lines

drivers/firmware/dmi-id.c +3 -2

@@ -103,8 +103,12 @@ static ssize_t get_modalias(char *buffer
 	char *p;
 	const struct mafield *f;
 
-	strcpy(buffer, "dmi");
-	p = buffer + 3; left = buffer_size - 4;
+	l = strscpy(buffer, "dmi", buffer_size);
+	if (l < 0)
+		return 0;

If function get_modalias() now has a return convention, it should be documented. But I see a problem which is that the return convention isn't clear. It *may* now return 0 on buffer overrun, but not necessarily. The rest of the function is best-effort mode and will silently drop a part of the modalias string if it doesn't fit. This is not consistent.

This is not caused by your patch, but this could actually cause false positive matches, so it may be a good idea to fix it while you're here. And in my opinion the best thing to do is to return an error rather than an half-baked modalias string. If the string doesn't fit as a whole, it's going to cause trouble at some point anyway, so we better learn about it early and do something about it. And that would be consistent.

I'm also curious why you chose to return 0 on error, rather than the more conventional -1 or -ENOMEM?

drivers/firmware/dmi-id.c static ssize_t get_modalias(char *buffer · line 109

Quoted 2 lines

drivers/firmware/dmi-id.c +1 -0

+	p = buffer + l; left = buffer_size - l - 1;

Please split on separate lines for readability. I would also appreciate a comment explaining that the "- 1" is to leave room for the trailing ":" (if I understand that right).

Quoted 4 lines

drivers/firmware/dmi-id.c +2 -0

+	if (left < 0)
+		return 0;
 

drivers/firmware/dmi-id.c static ssize_t sys_dmi_modalias_show(str · line 142

Quoted 8 lines

drivers/firmware/dmi-id.c +1 -1

 	for (f = fields; f->prefix && left > 0; f++) {
 		const char *c;

@@ -135,7 +139,7 @@ static ssize_t sys_dmi_modalias_show(str
 				     struct device_attribute *attr, char *page)
 {
 	ssize_t r;
-	r = get_modalias(page, PAGE_SIZE-1);
+	r = get_modalias(page, PAGE_SIZE-2);

Why? As I read the code, get_modalias() returns the length of the string, excluding the trailing '\0'. So it will be, at most, one less than the buffer size we passed. So if we pass PAGE_SIZE-1, r is at most PAGE_SIZE-2, which leaves exactly the 2 bytes we need for the two lines of code below. Am I missing something?

(The last line of get_modalias would probably be clearer as:

	return (p + 1) - buffer;

or if p was increased beforehand to actually point to the end of the string.)

drivers/firmware/dmi-id.c static int dmi_dev_uevent(const struct d · line 171

Quoted 10 lines

drivers/firmware/dmi-id.c +1 -1

 	page[r] = '\n';
 	page[r+1] = 0;
 	return r+1;

@@ -163,7 +167,7 @@ static int dmi_dev_uevent(const struct d
 		return -ENOMEM;
 	len = get_modalias(&env->buf[env->buflen - 1],
 			   sizeof(env->buf) - env->buflen);
-	if (len >= (sizeof(env->buf) - env->buflen))
+	if (!len || len >= (sizeof(env->buf) - env->buflen))
 		return -ENOMEM;

I do not like the fact that we check whether get_modalias() returns an error here, and not in sys_dmi_modalias_show(). This is inconsistent. IMHO both functions should check the return value and return an error code on failure.

As a side note, I can't see how the second condition could be true. We pass the buffer size to get_modalias() exactly to make sure that it won't write past the buffer's end.

drivers/firmware/dmi-id.c static int dmi_dev_uevent(const struct d · line 173

Quoted 3 lines

drivers/firmware/dmi-id.c +0 -0

 	env->buflen += len;
 	return 0;