Re: i386: kernel overflow in syscall

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 27/12/2016 à 20:08, David Laight a écrit :
> On Sun, Sep 11, 2016 at 11:51:27AM +0200, Maxime Villard wrote:
> > When syscalling on i386, the kernel copies in the sycall args with a special
> > function [1]. The third argument is the size of the userland area. In
> > x86_copyargs, however, you can see [2] that the area is copied by two chunks
> > of 16 and 20 bytes, which means that if a syscall has a 4-byte-sized
> > argument, the kernel still copies 16 bytes.
> >
> > The problem is that the VM_MAXUSER_ADDRESS check (in charge of making sure
> > we don't start reading kernel space) is done on the third argument, and not
> > the real copied size.
> >
> > It is easy to create a condition where %esp+4+arg_size <= VM_MAXUSER_ADDRES
> > and %esp+4+roundup(arg_size, 16) > VM_MAXUSER_ADDRESS. In such a case, the
> > userland check succeeds, and the kernel starts copying its own memory.
> >
> > Without going into terrible details, it is possible for userland to make
> > sure VM_MAXUSER_ADDRESS+delta is readable, and therefore, x86_copyargs will
> > succeed, and some args will contain some slots of the page tree.
> >
> > This bug seems harmless, but I guess it is still possible to leak kernel
> > data to userland. I wrote [3] quickly, which triggers such a condition and
> > goes unseen.
> >
> > x86_copyargs is supposed to be an 'optimized' copyin. While the design of
> > this function is obviously wrong, I would simply suggest rounding up the
> > size of the third argument [4].
> >
> > Just sayin
> >
> > [1] https://nxr.netbsd.org/xref/src/sys/arch/x86/x86/syscall.c#150
> > [2] https://nxr.netbsd.org/xref/src/sys/arch/i386/i386/copy.S#679
> > [3] http://m00nbsd.net/garbage/sysover/sysover.c
> > [4] http://m00nbsd.net/garbage/sysover/syscall.diff
>
> It was completely harmless.
> The values being read are the system call parameters, the length
> check was against the number of parameters the kernel code would
> look at.

Yes, exactly: it is a read overflow, not a write overflow. After re-reading my
mail I don't think I somehow suggested the contrary; and it indeed is harmless.
Still, it was a bug.

> So the extra values should never even be looked at - never mind
> leaked back to userspace.

As I said I think there would have been a way to leak it back to userland, in
which case userland would have been able to determine the physical layout of
its first VAs. But anyway, it wouldn't have been dramatic either.

> Patch [4] was completely wrong - it increased the size of the copy.
> Checking the max size (as commited) is ok though.

Yes, I later figured out it was wrong, and that's precisely why I ended up
committing a different patch - without sending an update on tech-kern, since
no one had answered, so not sure it was worth it.

I guess I'll have to remove all my misleading garbage stuff.