[GIT] Networking

On Fri, Oct 29, 2010 at 12:59 PM, David Miller <davem@davemloft.net> wrote:
>
> This has the verify_iovec() INT_MAX limiter change as well as:

I think you'd want this as well, to make sure that sendto/recvfrom
don't generate invalid iovecs.

Feel free to add my sign-off (or just commit it as yourself) after
giving it some testing.

NOTE! On thing that struck me is that the VFS layer does the
"access_ok()" on the pre-truncated size and pointer pair, and I think
that is the correct thing to do. However, the socket layer (and this
patch) just truncates the size, so even if the copy is then done
correctly with the proper user access checking, it will not check that
the whole original buffer was valid - only that the buffer it fills in
is valid.

Now, this is not a security issue (since we're just not checking stuff
that isn't getting filled in), but I think it's a QoI issue - it
allows users to successfully pass in bogus buffers with huge sizes,
and then if the thing only reads a few bytes it will all be ok.

That's not a new thing: the old code may not have truncated the sizes,
but if you pass in a 2GB buffer size, 99.999% of all socket read calls
obviously won't ever fill that 2GB, but will happily return with
whatever is there in the socket now (especially with nonblocking IO
etc). But I do wonder if we shouldn't do the access_ok() on the whole
buffer, as a way to keep user code honest.

                  Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Fri, 29 Oct 2010 14:41:03 -0700

> On Fri, Oct 29, 2010 at 12:59 PM, David Miller <davem@davemloft.net> wrote:
>>
>> This has the verify_iovec() INT_MAX limiter change as well as:
> 
> I think you'd want this as well, to make sure that sendto/recvfrom
> don't generate invalid iovecs.
> 
> Feel free to add my sign-off (or just commit it as yourself) after
> giving it some testing.

Done, thanks.

> NOTE! On thing that struck me is that the VFS layer does the
> "access_ok()" on the pre-truncated size and pointer pair, and I think
> that is the correct thing to do. However, the socket layer (and this
> patch) just truncates the size, so even if the copy is then done
> correctly with the proper user access checking, it will not check that
> the whole original buffer was valid - only that the buffer it fills in
> is valid.
> 
> Now, this is not a security issue (since we're just not checking stuff
> that isn't getting filled in), but I think it's a QoI issue - it
> allows users to successfully pass in bogus buffers with huge sizes,
> and then if the thing only reads a few bytes it will all be ok.
> 
> That's not a new thing: the old code may not have truncated the sizes,
> but if you pass in a 2GB buffer size, 99.999% of all socket read calls
> obviously won't ever fill that 2GB, but will happily return with
> whatever is there in the socket now (especially with nonblocking IO
> etc). But I do wonder if we shouldn't do the access_ok() on the whole
> buffer, as a way to keep user code honest.

I honestly don't think it matters.

I suppose we could put the access_ok() check right before these
single-buffer truncations, and then also in the per-iovec check of
{compat_}verify_iovec().

But what would all of that really give us?

Ingrained in datagram socket handling is the idea that the whole
buffer will be processed, and for stream sockets partial buffer
transfers are OK.

And I think this aligns with how we implement and check things
right now.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html