linux-kernel - Re: [PATCH v3] sched/core: Fix uclamp ABI bug, clean up and robustify sched_read

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]

Date:   Wed, 4 Sep 2019 19:53:57 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Thadeu Lima de Souza Cascardo <cascardo@...onical.com>
Cc:     Dietmar Eggemann <dietmar.eggemann@....com>,
        linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Patrick Bellasi <patrick.bellasi@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnaldo Carvalho de Melo <acme@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jiri Olsa <jolsa@...hat.com>
Subject: Re: [PATCH v3] sched/core: Fix uclamp ABI bug, clean up and
 robustify sched_read_attr() ABI logic and code

* Thadeu Lima de Souza Cascardo <cascardo@...onical.com> wrote:

> There is one odd behaviour now, which is whenever size is set between 
> VER0 and VER1, we will have a partial struct filled up, instead of 
> getting E2BIG or EINVAL.

Well, that's pretty much by design: user-space is asking for 'usize' 
bytes of information, and the kernel provides it if it can.

This way the kernel can keep backwards compatibility indefinitely, 
without new kernels having to be aware of the zillions of prior ABI 
versions that were due to slow expansion of the ABI. (This actually 
happened to the perf ABI, which was expanded dozens of times already.)

> > +static int
> > +sched_attr_copy_to_user(struct sched_attr __user *uattr,
> > +			struct sched_attr *kattr,
> > +			unsigned int usize)
> >  {
> > -	int ret;
> > +	unsigned int ksize = sizeof(*kattr);
> >  
> > -	if (!access_ok(uattr, usize))
> > +	if (!access_ok(uattr, ksize))
> >  		return -EFAULT;
> >  
> 
> I believe this should be either usize or kattr->size and moved below 
> (or just reuse min(usize,ksize)).
> 
> Otherwise, an old binary that uses the old ABI (that is, VER0 as size) 
> may get EFAULT here. This should almost never in practice. I tried, but 
> I could hardly use an address that would fail access_ok. But, 
> theoretically, that still breaks ABI.

I agree that this is mostly theoretical, as currently these sizes are 
limited between VER0 and PAGE_SIZE - and hardly anyone puts these 
structures near the very end of virtual memory.

But checking 'usize' is arguably the more correct value, as that's the 
size of the user-space buffer. I've done this change in the latest 
version of the fix.

Thanks,

	Ingo