linux-kernel - Re: [PATCH] usbip: handle length at sysfs show() functions

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: <20110607213418.GB11102@kroah.com>
Date:	Tue, 7 Jun 2011 14:34:18 -0700
From:	Greg KH <greg@...ah.com>
To:	Németh Márton <nm127@...email.hu>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>,
	Matt Mooney <mfm@...eddisk.com>,
	Kulikov Vasiliy <segooon@...il.com>,
	Endre Kollar <taxy443@...il.com>,
	Arjan Mels <arjan.mels@....net>,
	Ilia Mirkin <imirkin@...m.mit.edu>,
	David Chang <dchang@...ell.com>,
	Himanshu Chauhan <hschauhan@...ltrace.org>,
	Max Vozeler <max@...eler.com>, Arnd Bergmann <arnd@...db.de>,
	usbip-devel@...ts.sourceforge.net, devel@...verdev.osuosl.org,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usbip: handle length at sysfs show() functions

On Wed, Jun 01, 2011 at 07:14:07AM +0200, Németh Márton wrote:
> From: Márton Németh <nm127@...email.hu>
> 
> The sysfs show() functions shall return the actual content length of
> the result buffer. According to Documentation/filesystems/sysfs.txt:215
> the scnprintf() function is preferred.
> 
> See also the article titled "snprintf() confusion" at
> http://lwn.net/Articles/69419/ .
> 
> Signed-off-by: Márton Németh <nm127@...email.hu>
> ---
> diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
> index 6e99ec8..af5f107 100644
> --- a/drivers/staging/usbip/stub_dev.c
> +++ b/drivers/staging/usbip/stub_dev.c
> @@ -80,7 +80,7 @@ static ssize_t show_status(struct device *dev, struct device_attribute *attr,
>  	status = sdev->ud.status;
>  	spin_unlock(&sdev->ud.lock);
> 
> -	return snprintf(buf, PAGE_SIZE, "%d\n", status);
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", status);

Actually, this can be changed to just be:
	return sprintf(buf, "%d\n", status);
as obviously an integer will never be bigger than the sysfs buffer.

Good rule of thumb, if you care about the size of the sysfs buffer, you
are doing something wrong.

Case in point:

> --- a/drivers/staging/usbip/stub_main.c
> +++ b/drivers/staging/usbip/stub_main.c
> @@ -79,18 +79,22 @@ static ssize_t show_match_busid(struct device_driver *drv, char *buf)
>  {
>  	int i;
>  	char *out = buf;
> +	int count = 0;
> 
>  	spin_lock(&busid_table_lock);
> 
>  	for (i = 0; i < MAX_BUSID; i++)
> -		if (busid_table[i].name[0])
> -			out += sprintf(out, "%s ", busid_table[i].name);
> +		if (busid_table[i].name[0]) {
> +			count += scnprintf(out, PAGE_SIZE - count,
> +					"%s ", busid_table[i].name);
> +			out = buf + count;
> +		}

Here we are doing lots of work to try to put more than one value in the
sysfs file, and return the proper data to the kernel about how big the
buffer we used.

That's wrong, and violates the "one value per file" sysfs rule, so that
should be fixed instead of trying to change the sprintf() call.

Same goes for other places in this patch.

Care to redo that instead?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/