netdev - Re: [RFC PATCH v3 02/20] tcp: don't allow non-devmem originated ppiov

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9f5ea0cb-215a-4b43-92dc-d306015c8c7a@gmail.com>
Date: Wed, 20 Dec 2023 01:29:08 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: Mina Almasry <almasrymina@...gle.com>, David Wei <dw@...idwei.uk>
Cc: io-uring@...r.kernel.org, netdev@...r.kernel.org,
 Jens Axboe <axboe@...nel.dk>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jesper Dangaard Brouer
 <hawk@...nel.org>, David Ahern <dsahern@...nel.org>
Subject: Re: [RFC PATCH v3 02/20] tcp: don't allow non-devmem originated ppiov

On 12/19/23 23:24, Mina Almasry wrote:
> On Tue, Dec 19, 2023 at 1:04 PM David Wei <dw@...idwei.uk> wrote:
>>
>> From: Pavel Begunkov <asml.silence@...il.com>
>>
>> NOT FOR UPSTREAM
>>
>> There will be more users of struct page_pool_iov, and ppiovs from one
>> subsystem must not be used by another. That should never happen for any
>> sane application, but we need to enforce it in case of bufs and/or
>> malicious users.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
>> Signed-off-by: David Wei <dw@...idwei.uk>
>> ---
>>   net/ipv4/tcp.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index 33a8bb63fbf5..9c6b18eebb5b 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -2384,6 +2384,13 @@ static int tcp_recvmsg_devmem(const struct sock *sk, const struct sk_buff *skb,
>>                          }
>>
>>                          ppiov = skb_frag_page_pool_iov(frag);
>> +
>> +                       /* Disallow non devmem owned buffers */
>> +                       if (ppiov->pp->p.memory_provider != PP_MP_DMABUF_DEVMEM) {
>> +                               err = -ENODEV;
>> +                               goto out;
>> +                       }
>> +
> 
> Instead of this, I maybe recommend modifying the skb->dmabuf flag? My
> mental model is that flag means all the frags in the skb are

That's a good point, we need to separate them, and I have it in my
todo list.

> specifically dmabuf, not general ppiovs or net_iovs. Is it possible to
> add skb->io_uring or something?

->io_uring flag is not feasible, converting ->devmem into a type
{page,devmem,iouring} is better but not great either.

> If that bloats the skb headers, then maybe we need another place to
> put this flag. Maybe the [page_pool|net]_iov should declare whether
> it's dmabuf or otherwise, and we can check frag[0] and assume all

ppiov->pp should be enough, either not mixing buffers from different
pools or comparing pp->ops or some pp->type.

> frags are the same as frag0.

I think I like this one the most. I think David Ahern mentioned
before, but would be nice having it on per frag basis and kill
->devmem flag. That would still stop collapsing if frags are
from different pools or so.

> But IMO the page pool internals should not leak into the
> implementation of generic tcp stack functions.

-- 
Pavel Begunkov