linux-kernel - Re: [PATCH] firmware: dmi: Respect buffer size in get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250313194145.284d7815@endymion>
Date: Thu, 13 Mar 2025 19:41:45 +0100
From: Jean Delvare <jdelvare@...e.de>
To: Cyrill Gorcunov <gorcunov@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] firmware: dmi: Respect buffer size in get_modalias

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@...e.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@...il.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