linux-kernel - Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32

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]

Date:	Wed, 19 Aug 2015 13:47:11 +0300
From:	Alexey Dobriyan <adobriyan@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Dongsu Park <dpark@...teo.net>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Peter Hurley <peter@...leysoftware.com>,
	Josh Triplett <josh@...htriplett.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	David Howells <dhowells@...hat.com>,
	Alban Crequy <alban@...ocode.com>
Subject: Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

On Wed, Aug 19, 2015 at 10:47 AM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Wed, 19 Aug 2015 09:24:31 +0200 Dongsu Park <dpark@...teo.net> wrote:
>
>> >     unsigned long uidl;
>> >
>> >     rc = kstrtoul(uidstr, 0, &uidl);
>> >     uidval = uidl;
>>
>> That's a good point. I'll do it.
>>
>> > > +                 if (rc)
>> > >                           return -EINVAL;
>> >
>> > I don't get it.  From my reading, kstrtouint->parse_integer() returns
>> > "number of characters parsed or -E".  So this code won't work.  But
>> > presumably it *does* work, so why?
>>
>> It's probably because kstrtouint() returns just 0 on success.
>> That's what functions in the call chain of kstrtouint() -> kstrtoull() ->
>> _kstrtoull() -> _parse_integer() are actually doing.
>> _parse_integer() actually returns rv, i.e. number of characters parsed.
>> But after that, if there's no error, _kstrtoull() simply returns 0.
>
> whoa, wait, I was looking at the -mm tree which changes kstrtouint():
>
> static inline int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res)
> {
>         return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
> }
>
> and
>
>  * Return number of characters parsed or -E.
>  ...
>  */
> #define parse_integer(s, base, val)     \
>
>
>
> Alexey, doesn't this mean that code which does
>
>         if (kstrtouint(...))
>                 return -EFOO;
>
> will break?

Nothing is supposed to break (even between patches in that series).
kstrto*() still returns 0 on success or -EINVAL/-ERANGE on error.

Commenting on other things in this thread:

> This assumes that the architecture/config
> uses a uint for uid_t. We have no business
> assuming this - it's an opaque type for a reason.
> It would be safer to do
>
> unsigned long uidl;
>
> rc = kstrtoul(uidstr, 0, &uidl);
> uidval = uidl;

This code is not safe at all because unsigned long is wider
than unsigned int. "4294967296" will be silently parsed as 0.
kstrtou32() should be used in this case. Yes, the names
do not match, but C types do. If uid_t as a type is changed,
compiler will notice immediately making kstrtou32() more safe.

> kstrtouint() likes to return -ERANGE when things go
> wrong. ERANGE means "Math result not representable",
> which is a nonsenscal error code in this context.

ERANGE is there to distinguish between "invalid" and
"valid but too big". Typical integer parsing code will accept
289367492873894273894729837428937489273
(a _very_ common bug).
C doesn't have bignums, so ERANGE exists.

And to teach people that -EINVAL is not the only error value,
so they won't hardcode it because in the future we might add
new error value because who knows why.

> PARSE_INTEGER_NEWLINE means more than
> just accepting a trailing newline.

Well, maybe it is misnamed, but it is internal implementation
detail. People aren't supposed to use it directly.
Name can be changed if it is so disturbing.
--
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/