linux-kernel - Re: [PATCH 04/10] sscanf: fix overflow

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:	Tue, 05 May 2015 11:51:55 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Alexey Dobriyan <adobriyan@...il.com>
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/10] sscanf: fix overflow

On Sat, May 02 2015, Alexey Dobriyan <adobriyan@...il.com> wrote:

> Fun fact:
>
> 	uint8_t val;
> 	sscanf("256", "%hhu", &val);
>
> will return 1 (as it should), and make val=0 (as it should not).
>

What do you base these "should" and "should not" on? Both C99 and POSIX
say that the behaviour is undefined - the kernel can obviously define
its own semantics for scanf, but what do you think they should be?

If we want to correctly handle overflow, the only sane way is to make
sscanf return 0 in the above case (no conversions done). This also
seems to be what your patch does, but then I'm confused by your
first "as it should".

> Apart from correctness, patch allows to remove checks and switch
> to proper types in several (most?) cases:
>
> 	grep -e 'scanf.*%[0-9]\+[dioux]' -n -r .
>
> Such checks can be incorrect too, checking for 3 digits with %3u
> for parsing uint8_t is not enough.

Yeah, and it may be too much; sscanf("0042", "%hhu", &val") should give
42, not 4. I agree that one should be able to rely on scanf doing range
checking as part of matching.

Actually, I think one should go through all the callers of sscanf which
use a field width with an integer conversion and see if we can get rid
of it, and then rip it away from the sscanf implementation. Otherwise
there's another bug which would need fixing, namely

int x;
char rest[50];
sscanf("12345678901234567890", "%3d%s", &x, rest)

should successfully return 2 (storing 123 in x), but it can't when the
strategy is to convert as much as possible (which may then give an early
failure due to overflow), then divide by 10 until we haven't consumed
more than we're allowed.

Rasmus
--
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/