Re: error in msync(2) or am I missing something?

Linus Torvalds (torvalds@transmeta.com)
4 Sep 1998 17:30:05 GMT


In article <Pine.LNX.4.02.9809041447310.5239-100000@einstein.london.sco.com>,
Tigran Aivazian <tigran@sco.COM> wrote:
>Hello guys,
>
>In the msync(2) (source mm/filemap.c/sys_msync()) function there is a piece of
>code:
>
>len = (len + ~PAGE_MASK) & PAGE_MASK;
>end = start + len;
>if (end < start)
> goto out;
>if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
> goto out;
>error = 0;
>if (end == start)
> goto out;
>
>Now, len is size_t (unsigned int) so it can hardly ever become negative. So,
>can't one simply rewrite the code as:

[ removed so that people don't get ideas ;-]

It's not that "len" can be negative, it's about wraparound. The "end <
start" bit is essentially testing the carry from the (unsigned) addition
one line above it. In fact, a good compiler will notice that, and
actually do the addition and the compare with a "add + jc" combination.

(There is no notion of carry in normal C, which is why it has to be done
this way if you want to be portable).

So you have three cases:
- wrapping addresses: bad, and should return an error
- an empty area (ok - return zero without doing anything)
- an actual potentially valid area (do some real work)

Note that address wrap-around is a _really_ common security issue in
operating systems. Linux has had its share, but probably fewer than
most because I and others have been so aware of it. If you don't check
for wrap-around, it is often fairly easy to fool a system into believing
you are giving it a good address range, even when you are not.

For example, assume that you have an OS that wants to restrict all user
pointers to the low virtual memory area (fairly common), and you're
given a range. The _wrong_ way to do this is to have something like

if (start + len > limit)
error

because that leaves you open to a malicious user that passes in an
address inside your protected region, but makes sure that the length is
big enough that the addition will wrap.

So you should do something like

if (start + len < start || start + len > limit)
error

where the first part guarantees that it doesn't wrap, and the second
part guarantees that all of the area is below the actual limit (and
again, a good compiler will actually turn the above into something like

/* start in %eax, len in %edx */
add %eax,%edx
jc bad
cmpl limit,%eax
ja bad

so it's actually only one extra check against carry in addition to the
limit check.

Look into asm-i386/uaccess.h for alternative ways of doing ranges right
by explicitly using the carry bit. I don't know if it's any better, but
it's different, and as such could be instructive.

Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/faq.html