linux-kernel - Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs

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: <4bef099f-b2e1-0327-271d-75662b366c07@rasmusvillemoes.dk>
Date:   Tue, 21 Aug 2018 14:43:27 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>, Martin Wilck <mwilck@...e.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and
 sysfs_strlcpy()

On 2018-08-21 11:50, Sergey Senozhatsky wrote:
> Hi Rasmus,
> 
> On (08/21/18 09:59), Rasmus Villemoes wrote:
>>> +char *sysfs_strncpy(char *dest, const char *src, size_t count)
>>> +{
>>> +	char *c;
>>> +
>>> +	strncpy(dest, skip_spaces(src), count);
>>
>> I'd like to see where and how you'd use this, but I'm very skeptical of
>> count being used both for the size of the dest buffer as well as an
>> essentially random argument to strncpy - if count is also the maximum
>> number of bytes to read from the src, you'd need to take the
>> skip_spaces() into account, because there are not count bytes left after
>> that...
>> And if src is not necessarily nul-terminated, skip_spaces() by
>> itself is wrong.
> 
> I think that sysfs input is always properly NULL-terminated. It may or
> may not contain \n, but \0 is expected to be there.

Maybe. But it shouldn't hurt to make it accept a src size (the caller
can always pass -1 is he's sure the input is nul-terminated) - I'm
thinking a new interface might also have uses outside sysfs.

>> Moreover, I don't think we should add more users or wrappers for strncpy
>> - I highly doubt the sysfs users you have in mind want the "fill the
>> rest of the buffer with '\0'" nor the "not enough room for a terminating
>> '\0'? Oh well, what could possibly go wrong" semantics.
> 
> The reason I added both strncpy() and strlcpy() was that there are lots
> of sysfs ->store() callbacks which use strncpy().
> 
> E.g.
> 	channel_dimm_label_store()
> 	dimmdev_label_store()
> 	pmbus_add_label()
> 	axp20x_store_attr()
> 	cmdline_store()

Hm, do any of those actually want the skipping of leading whitespace?
pmbus_add_label doesn't even seem to kill a trailing newline?

It's probably hard to create one interface that will work for all cases.
Some are happy with truncating the user input, some refuse to do the
copy if truncation might happen, some probably do the copy (clobbering
what was there) but then detect the truncation somehow and return EINVAL
or whatever. Also, some copy to a pre-existing buffer, while others
kmalloc() a buffer and populate it with strncpy. The latter would
probably be better served using kstrndup or a small variant.

>> I think you're too focused on making wrappers around str[ln]cpy
>> preserving parts of those functions' API. Instead, try to figure out
>> what sysfs users actually want, name the functions after that, and then
>> whether they use strncpy or sprintf or strscpy internally is completely
>> irrelevant.
> 
> [Good] point, that's why the patch is in RFC stage: to figure out
> what do we actually want.

Dunno, looking at your examples above I'm a bit more skeptical that
there is a lot of common ground. But of course there are hundreds of
other store callbacks, so surely one can simplify some with a new helper.

Rasmus