Hello Vegard,
On 08/18/2016 07:34 AM, Vegard Nossum wrote:
> On 08/17/2016 10:02 AM, Michael Kerrisk (man-pages) wrote:
>> On 08/17/2016 10:00 AM, Vegard Nossum wrote:
>>>>> Isn't there also a race where two or more concurrent pipe()/fnctl()
>>>>> calls can together push us over the limits before the accounting is done?
>>>>
>>>> I guess there is!
>>>>
>>>>> I think there really ought to be a check after doing the accounting if
>>>>> we really want to be meticulous here.
>>>>
>>>> Let me confirm what I understand from your comment: because of the race,
>>>> then a user could subvert the checks and allocate an arbitrary amount
>>>> of kernel memory for pipes. Right?
>
> Forgot to respond to this earlier, sorry. It wouldn't be an arbitrary
> amount exactly, as it would still be limited by the number of processes
> you could get to allocate a pipe at exactly the right moment (since each
> pipe would still be bound by the limit by itself).
>
>>>> I'm not sure what you mean by "a check after doing the accounting". Is not the
>>>> only solution here some kind of lock around the check+accounting steps?
>>>
>>> Instead of doing atomic_long_read() in the check + atomic_long_add() for
>>> accounting we could do a single speculative atomic_long_add_return() and
>>> then if it goes above the limit we can lower it again with atomic_sub()
>>> when aborting the operation (if it doesn't go above the limit we don't
>>> need to do anything).
>>
>> So, would that mean something like the following (where I've moved
>> some checks from pipe_fcntl() to pipe_set_size()):
> [...]
>> static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long nr_pages)
>> {
>> struct pipe_buffer *bufs;
>> unsigned int size;
>> long ret = 0;
>>
>> size = nr_pages * PAGE_SIZE;
>> account_pipe_buffers(pipe, pipe->buffers, nr_pages);
>>
>> /*
>> * If trying to increase the pipe capacity, check that an
>> * unprivileged user is not trying to exceed various limits.
>> * (Decreasing the pipe capacity is always permitted, even
>> * if the user is currently over a limit.)
>> */
>> if (nr_pages > pipe->buffers) {
>> if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
>> ret = -EPERM;
>> } else if ((too_many_pipe_buffers_hard(pipe->user, 0) ||
>> too_many_pipe_buffers_soft(pipe->user, 0)) &&
>> !capable(CAP_SYS_RESOURCE) &&
>> !capable(CAP_SYS_ADMIN)) {
>> ret = -EPERM;
>> }
>> }
>>
>> /*
>> * If we exceeded a limit, revert the accounting and go no further
>> */
>> if (ret) {
>> account_pipe_buffers(pipe, nr_pages, pipe->buffers);
>> return ret;
>> }
> [...]
>>
>> Seem okay? Probably, some analogous fix is required in alloc_pipe_info()
>> when creating a pipe(?).
>
> I suppose that works. You could still have account_pipe_buffers() return
> the value of the new &pipe->user->pipe_bufs directly like I suggested in
> my previous email to avoid the extra atomic accesses in
> too_many_pipe_buffers_{soft,hard}() but I guess nobody *really* cares
> that much about performance here.
>
> I am no authority on this code but it looks safe and sound to me.
Okay -- thanks. I'll look at tightening this patch.
And, do you agree that something similar is required for alloc_pipe_info()
when creating a pipe?
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html