netdev - Re: [PATCH] xsk: Add missing overflow check in xdp_umem

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]

Date:   Tue, 7 Mar 2023 19:58:51 +0100
From:   Kal Cutter Conley <kal.conley@...tris.com>
To:     Alexander Lobakin <aleksander.lobakin@...el.com>
Cc:     Björn Töpel <bjorn@...nel.org>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        netdev@...r.kernel.org, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xsk: Add missing overflow check in xdp_umem_reg

> The RCT declaration style is messed up in the whole block. Please move
> lines around, there's nothing wrong in that.

I think I figured out what this is. Is this preference documented
somewhere? I will fix it.

>
> >       int err;
> >
> >       if (chunk_size < XDP_UMEM_MIN_CHUNK_SIZE || chunk_size > PAGE_SIZE) {
> > @@ -188,8 +189,8 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
> >       if (npgs > U32_MAX)
> >               return -EINVAL;
> >
> > -     chunks = (unsigned int)div_u64_rem(size, chunk_size, &chunks_rem);
> > -     if (chunks == 0)
> > +     chunks = div_u64_rem(size, chunk_size, &chunks_rem);
> > +     if (chunks == 0 || chunks > U32_MAX)
>
> You can change the first cond to `!chunks` while at it, it's more
> preferred than `== 0`.

If you want, I can change it. I generally like to keep unrelated
changes to a minimum.

>
> >               return -EINVAL;
>
> Do you have any particular bugs that the current code leads to? Or it's
> just something that might hypothetically happen?

If the UMEM is large enough, the code is broke. Maybe it can be
exploited somehow? It should be checked for exactly the same reasons
as `npgs` right above it.

>
> >
> >       if (!unaligned_chunks && chunks_rem)
> > @@ -201,7 +202,7 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
> >       umem->size = size;
> >       umem->headroom = headroom;
> >       umem->chunk_size = chunk_size;
> > -     umem->chunks = chunks;
> > +     umem->chunks = (u32)chunks;
>
> You already checked @chunks fits into 32 bits, so the cast can be
> omitted here, it's redundant.

I made it consistent with the line right below it. It seems like the
cast may improve readability since it makes it known the truncation is
on purpose. I don't see how that is redundant with the safety check.
Should I change both lines?

>
> >       umem->npgs = (u32)npgs;
> >       umem->pgs = NULL;
> >       umem->user = NULL;
>
> Thanks,
> Olek

Kal