Re: [PATCH 1/2] prctl: take mmap sem for writing to protect against others

[Mateusz Guzik]

On Wed, Jan 06, 2016 at 03:05:09PM +0530, Anshuman Khandual wrote:
> On Wed, Jan 6, 2016 at 10:32 AM, Mateusz Guzik <mguzik@xxxxxxxxxx> wrote:
> > The code was taking the semaphore for reading, which does not protect
> > against readers nor concurrent modifications.
> 
> (down/up)_read does not protect against concurrent readers ?
> 

Maybe wording could be improved.

The point is, prctl is modifying various fields while having the
semaphore for reading. Other code, presumably wanting to read said
fields, can take semaphore for reading itsef in order to get a stable
copy. Except turns out it does not help here. Since the both the reader
and the writer did that, the reader can read stuff as it is changing.

> >
> > The problem could cause a sanity checks to fail in procfs's cmdline
> > reader, resulting in an OOPS.
> >
> 
> Can you explain this a bit and may be give some examples ?
> 

Given the above, let's consider a program moving around arg_start +
arg_end by few bytes while proc_pid_cmdline_read is being executed.

proc_pid_cmdline_read does:
        down_read(&mm->mmap_sem);
        arg_start = mm->arg_start;
        arg_end = mm->arg_end;
        env_start = mm->env_start;
        env_end = mm->env_end;
        up_read(&mm->mmap_sem);

Since the writer also has the semaphore only for reading, this function
can proceed to read stuff as it is being modified. In particular it can
read arg_start *prior* to modification and arg_end *after*. This trips
up BUG_ON(arg_start > arg_end).

That is, for the sake of argument, if the code is changing the pair from
0x2000 & 0x2020 to 0x1000 & 0x1020, someone else may read arg_start
0x2000 and arg_end 0x1020.

This code should showcase the problem:
http://people.redhat.com/~mguzik/misc/prctl.c

It is somewhat crude, you may ned to adjust hardcoded values for your
binary.

> > Note that some functions perform an unlocked read of various mm fields,
> > but they seem to be fine despite possible modificaton.
> 
> Those need to be fixed as well ?
> 

As stated earlier, these can live without changes, but that does not
look like a good idea.

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