Messages in this thread | | | Subject | Re: [PATCH] sched/core: check format and overflows in cgroup2 cpu.max | From | Konstantin Khlebnikov <> | Date | Wed, 6 Mar 2019 20:21:40 +0300 |
| |
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 "%[...]".
| |