MIME-Version: 1.0
References: <20230307172306.786657-1-kal.conley@dectris.com> <ee0aa756-4a9c-1d7a-4179-78024e41d37e@intel.com>
In-Reply-To: <ee0aa756-4a9c-1d7a-4179-78024e41d37e@intel.com>
From:   Kal Cutter Conley <kal.conley@dectris.com>
Date:   Tue, 7 Mar 2023 19:58:51 +0100
Message-ID: <CAHApi-kcvc2qB0D6dV7OG99FsnzAEa-rchOMfySkZ-E=EOh_4w@mail.gmail.com>
Subject: Re: [PATCH] xsk: Add missing overflow check in xdp_umem_reg
To:     Alexander Lobakin <aleksander.lobakin@intel.com>
Cc:     =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        Magnus Karlsson <magnus.karlsson@intel.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Jonathan Lemon <jonathan.lemon@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        netdev@vger.kernel.org, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> 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