[PATCH] firmware: dmi: Respect buffer size in get_modalias

[PATCH] firmware: dmi: Respect buffer size in get_modalias

[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.

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

CC: Jean Delvare <jdelvare@suse.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 drivers/firmware/dmi-id.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: linux-tip.git/drivers/firmware/dmi-id.c
===================================================================
--- linux-tip.git.orig/drivers/firmware/dmi-id.c
+++ linux-tip.git/drivers/firmware/dmi-id.c
@@ -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;
+	p = buffer + l; left = buffer_size - l - 1;
+	if (left < 0)
+		return 0;
 
 	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);
 	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;
 	env->buflen += len;
 	return 0;

Re: [PATCH] firmware: dmi: Respect buffer size in get_modalias

[Cyrill Gorcunov]

On Thu, Feb 20, 2025 at 11:53:28PM +0300, Cyrill Gorcunov wrote:
> 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 <jdelvare@suse.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

Ping?

Re: [PATCH] firmware: dmi: Respect buffer size in get_modalias

[Jean Delvare]

Hi Cyrill,

On Thu, 20 Feb 2025 23:53:28 +0300, Cyrill Gorcunov wrote:
> 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?

> 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.

> CC: Jean Delvare <jdelvare@suse.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  drivers/firmware/dmi-id.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> Index: linux-tip.git/drivers/firmware/dmi-id.c
> ===================================================================
> --- linux-tip.git.orig/drivers/firmware/dmi-id.c
> +++ linux-tip.git/drivers/firmware/dmi-id.c
> @@ -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?

> +	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).

> +	if (left < 0)
> +		return 0;
>  
>  	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.)

>  	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.

>  	env->buflen += len;
>  	return 0;

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] firmware: dmi: Respect buffer size in get_modalias

[Cyrill Gorcunov]

On Thu, Mar 13, 2025 at 07:41:45PM +0100, Jean Delvare wrote:
> Hi Cyrill,
> 
> On Thu, 20 Feb 2025 23:53:28 +0300, Cyrill Gorcunov wrote:
> > 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?

Sure it would. Still at the moment a "page" for sysfs buffer is rather
hardcoded and a huge amount of other code relies on this size, some brave
soul need to spend vast slab of time to review each sysfs writer. Actually
I would not touch this code if get_modalias been used by sysfs only.

> > 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.
... 
> > --- linux-tip.git.orig/drivers/firmware/dmi-id.c
> > +++ linux-tip.git/drivers/firmware/dmi-id.c
> > @@ -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.

The problem is... userspace. I'm not sure what is better -- return
an error and empty data or trimmed strings. If you prefer error instead
of course I can make it so.

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

To match snprintf api when zero size is passed ('cause for udev we will
pass 0 if buffer is already exhausted).

> 
> > +	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).

Exactly, I was scratching my head first too figuring out why additional
char is needed here.

> 
> > +	if (left < 0)
> > +		return 0;
> >  
> >  	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?

Yeah, I managed to overcounting here, page_size-1 should be enough.

> 
> (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.)
> 
> >  	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.

I just added !len here, which didn't make code anyhow better, good point!

> 
> >  	env->buflen += len;
> >  	return 0;

So Jean, do you think it worth to rework this patch and add an error in case of overflow?
To be honest I made this patch simply because I miscounted PAGE_SIZE-1 case here (probably
should stop reading code at deep nights :) Since we never ever hit buffer overflow so far
neither in sysfs or udev it obviously not critical. Still if you think it would worth
to address a potential problem I'll rework it and resend addressing your comments.

	Cyrill

[PATCH v2] firmware: dmi: Respect buffer size in get_modalias

[Cyrill Gorcunov]

When we collect data from DMI info the "dmi" prefix is copied unconditionally
which may result in buffer overflow in case of filling uevent environment.
Thus lets use strscpy() helper instead. Same time make all get_modalias()
callers to handler error.

CC: Jean Delvare <jdelvare@suse.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
v2:
 - add comment about reserving space for suffix
 - check for error in callers

 drivers/firmware/dmi-id.c |   30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Index: linux-tip.git/drivers/firmware/dmi-id.c
===================================================================
--- linux-tip.git.orig/drivers/firmware/dmi-id.c
+++ linux-tip.git/drivers/firmware/dmi-id.c
@@ -103,8 +103,15 @@ 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 -ENOMEM;
+	p = buffer + l;
+
+	/* Reserve place for suffix */
+	left = buffer_size - l - 1;
+	if (left < 0)
+		return -ENOMEM;
 
 	for (f = fields; f->prefix && left > 0; f++) {
 		const char *c;
@@ -125,20 +132,21 @@ static ssize_t get_modalias(char *buffer
 		left -= l;
 	}
 
-	p[0] = ':';
-	p[1] = 0;
+	*p++ = ':';
+	*p = 0;
 
-	return p - buffer + 1;
+	return p - buffer;
 }
 
 static ssize_t sys_dmi_modalias_show(struct device *dev,
 				     struct device_attribute *attr, char *page)
 {
-	ssize_t r;
-	r = get_modalias(page, PAGE_SIZE-1);
-	page[r] = '\n';
-	page[r+1] = 0;
-	return r+1;
+	ssize_t r = get_modalias(page, PAGE_SIZE-1);
+	if (r > 0) {
+		page[r++] = '\n';
+		page[r] = 0;
+	}
+	return r;
 }
 
 static struct device_attribute sys_dmi_modalias_attr =
@@ -163,7 +171,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 < 0)
 		return -ENOMEM;
 	env->buflen += len;
 	return 0;