linux-kernel - Re: [PATCH] sched/core: check format and overflows in cgroup2 cpu.max

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] [day] [month] [year] [list]

Message-ID: <7c418d64-734e-000c-052a-f6fc59fdcf8e@yandex-team.ru>
Date:   Wed, 6 Mar 2019 20:21:40 +0300
From:   Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
To:     Peter Zijlstra <peterz@...radead.org>, Tejun Heo <tj@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Li Zefan <lizefan@...wei.com>,
        Johannes Weiner <hannes@...xchg.org>, cgroups@...r.kernel.org,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH] sched/core: check format and overflows in cgroup2 cpu.max

On 06.03.2019 19:48, Peter Zijlstra wrote:
> On Wed, Mar 06, 2019 at 08:11:54AM -0800, Tejun Heo wrote:
>> Hello, Konstantin.
>>
>> On Tue, Mar 05, 2019 at 08:03:24PM +0300, Konstantin Khlebnikov wrote:
>>>> Ditto as the blkio patch.  Unless there is a correctness problem, my
>>>> preference is towards keeping the parsing functions simple and I don't
>>>> think the kernel needs to play the role of strict input verifier here
>>>> as long as the only foot getting shot is the user's own.
>>>
>>> IMHO non-strict interface more likely hides bugs and could cause
>>> problems for future changes.
>>>
>>> Here is only only one fatal bug - buffer overflow in sscanf because
>>> %s has no limit.
>>
>> Ah, indeed.  Can you please post a patch to fix that problem first?

Done.
Please see [PATCH] sched/core: fix buffer overflow in cgroup2 property cpu.max

>>
>>> Strict validation could be done as more strict sscanf variant or
>>> some kind of extension for format string.
>>
>> I don't necessarily disagree with you; however, what often ends up
>> with these manually crafted parsing approach are 1. code which is
>> unnecessarily difficult to follow 2. different subset of validations
>> and parsing bugs (of course) everywhere.
>>
>> Given the above, I tend to lean towards dump sscanf() parsing.  If we
>> wanna improve the situation, I think the right thing to do is either
>> improving sscanf or introducing new helpers to parse these things
>> rather than hand-crafting each site.  It is really error-prone.

I'm playing with sscanf right now.

Both problems (integer overflows and matching end of string)
are relatively easy to fix without breaking sane compatibility.

> 
> Always use a field width specifier with %s.  Which is exactly what the
> proposed patch did IIRC.
> 
> Maybe that's something checkpatch could warn about.
> 

This could be done mandatory.
In-kernel sscanf always requires width for "%[...]".