Re: SECURITY PROBLEM: Any user can crash the kernel with TCP ZEROCOPY

[Eric Dumazet <edumazet@google.com>]

On Thu, Jan 25, 2024 at 3:18 AM zhangpeng (AS) <zhangpeng362@huawei.com> wrote:
>
> On 2024/1/24 18:11, Eric Dumazet wrote:
>
> > On Wed, Jan 24, 2024 at 10:30 AM zhangpeng (AS) <zhangpeng362@huawei.com> wrote:
> >>
> >> By using git-bisect, the patch that introduces this issue is 05255b823a617
> >> ("tcp: add TCP_ZEROCOPY_RECEIVE support for zerocopy receive."). v4.18-rc1.
> >>
> >> Currently, there are no other repro or c reproduction programs can reproduce
> >> the issue. The syz log used to reproduce the issue is as follows:
> >>
> >> r3 = socket$inet_tcp(0x2, 0x1, 0x0)
> >> mmap(&(0x7f0000ff9000/0x4000)=nil, 0x4000, 0x0, 0x12, r3, 0x0)
> >> r4 = socket$inet_tcp(0x2, 0x1, 0x0)
> >> bind$inet(r4, &(0x7f0000000000)={0x2, 0x4e24, @multicast1}, 0x10)
> >> connect$inet(r4, &(0x7f00000006c0)={0x2, 0x4e24, @empty}, 0x10)
> >> r5 = openat$dir(0xffffffffffffff9c, &(0x7f00000000c0)='./file0\x00',
> >> 0x181e42, 0x0)
> >> fallocate(r5, 0x0, 0x0, 0x85b8818)
> >> sendfile(r4, r5, 0x0, 0x3000)
> >> getsockopt$inet_tcp_TCP_ZEROCOPY_RECEIVE(r4, 0x6, 0x23,
> >> &(0x7f00000001c0)={&(0x7f0000ffb000/0x3000)=nil, 0x3000, 0x0, 0x0,
> >> 0x0, 0x0, 0x0, 0x0, 0x0}, &(0x7f0000000440)=0x10)
> >> r6 = openat$dir(0xffffffffffffff9c, &(0x7f00000000c0)='./file0\x00',
> >> 0x181e42, 0x0)
> >>
> > Could you try the following fix then ?
> >
> > (We also could remove the !skb_frag_off(frag) condition, as the
> > !PageCompound() is necessary it seems :/)
> >
> > Thanks a lot !
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 1baa484d21902d2492fc2830d960100dc09683bf..ee954ae7778a651a9da4de057e3bafe35a6e10d6
> > 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -1785,7 +1785,9 @@ static skb_frag_t *skb_advance_to_frag(struct
> > sk_buff *skb, u32 offset_skb,
> >
> >   static bool can_map_frag(const skb_frag_t *frag)
> >   {
> > -       return skb_frag_size(frag) == PAGE_SIZE && !skb_frag_off(frag);
> > +       return skb_frag_size(frag) == PAGE_SIZE &&
> > +              !skb_frag_off(frag) &&
> > +              !PageCompound(skb_frag_page(frag));
> >   }
> >
> >   static int find_next_mappable_frag(const skb_frag_t *frag,
>
> This patch doesn't fix this issue. The page cache that can trigger this issue
> doesn't necessarily need to be compound. 🙁

Ah, too bad :/

So the issue is that the page had a mapping. I am no mm expert,
I am not sure if we need to add more tests (like testing various
illegal page flags) ?

Can you test this ?

(I am still  converting the repro into C)

Thanks.

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1baa484d21902d2492fc2830d960100dc09683bf..2128015227a5066ea74b3911ecaefe7992da132f
100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1785,7 +1785,17 @@ static skb_frag_t *skb_advance_to_frag(struct
sk_buff *skb, u32 offset_skb,

 static bool can_map_frag(const skb_frag_t *frag)
 {
-       return skb_frag_size(frag) == PAGE_SIZE && !skb_frag_off(frag);
+       struct page *page;
+
+       if (skb_frag_size(frag) != PAGE_SIZE || skb_frag_off(frag))
+               return false;
+
+       page = skb_frag_page(frag);
+
+       if (PageCompound(page) || page->mapping)
+               return false;
+
+       return true;
 }

 static int find_next_mappable_frag(const skb_frag_t *frag,