Message-ID: <Z9NYLCdVfp2Nzet9@grain>
Date: Fri, 14 Mar 2025 01:11:56 +0300
From: Cyrill Gorcunov <gorcunov@...il.com>
To: Jean Delvare <jdelvare@...e.de>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] firmware: dmi: Respect buffer size in get_modalias

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

Powered by blists - more mailing lists