tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: i386: kernel overflow in syscall

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


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.

Home | Main Index | Thread Index | Old Index