| Message ID | 20230307172306.786657-1-kal.conley@dectris.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xsk: Add missing overflow check in xdp_umem_reg | expand |
From: Kal Conley <kal.conley@dectris.com> Date: Tue, 7 Mar 2023 18:23:06 +0100 > The number of chunks can overflow u32. Make sure to return -EINVAL on > overflow. > > Fixes: bbff2f321a86 ("xsk: new descriptor addressing scheme") > Signed-off-by: Kal Conley <kal.conley@dectris.com> > --- > net/xdp/xdp_umem.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c > index 4681e8e8ad94..f1aa79018ce8 100644 > --- a/net/xdp/xdp_umem.c > +++ b/net/xdp/xdp_umem.c > @@ -150,10 +150,11 @@ static int xdp_umem_account_pages(struct xdp_umem *umem) > > static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr) > { > - u32 npgs_rem, chunk_size = mr->chunk_size, headroom = mr->headroom; > + u32 chunk_size = mr->chunk_size, headroom = mr->headroom; > bool unaligned_chunks = mr->flags & XDP_UMEM_UNALIGNED_CHUNK_FLAG; > - u64 npgs, addr = mr->addr, size = mr->len; > - unsigned int chunks, chunks_rem; > + u64 addr = mr->addr, size = mr->len; > + u64 chunks, npgs; > + u32 chunks_rem, npgs_rem; The RCT declaration style is messed up in the whole block. Please move lines around, there's nothing wrong in that. > 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`. > return -EINVAL; Do you have any particular bugs that the current code leads to? Or it's just something that might hypothetically happen? > > 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. > umem->npgs = (u32)npgs; > umem->pgs = NULL; > umem->user = NULL; Thanks, Olek
> 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
From: Kal Conley <kal.conley@dectris.com> Date: Tue, 7 Mar 2023 19:58:51 +0100 >> 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. It's when you sort the declarations by the line length. I.e. short var a; longest var b; medium var c; => longest var b; medium var c; short var a; I think it's documented somewhere in the kernel. You can try grepping by "Reverse Christmas Tree". > >> >>> 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. You modify the line either way, so I don't see any reasons to keep the code as-is. It's clear that replacing `== 0` to `!chunks` won't change the logic anyhow. > >> >>> 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? I'd prefer to change both lines. You already check both @npgs and @chunks for being <= %U32_MAX and anyone can see it from the code, so the casts don't make anything more readable. > >> >>> umem->npgs = (u32)npgs; >>> umem->pgs = NULL; >>> umem->user = NULL; >> >> Thanks, >> Olek > > Kal Thanks, Olek
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c index 4681e8e8ad94..f1aa79018ce8 100644 --- a/net/xdp/xdp_umem.c +++ b/net/xdp/xdp_umem.c @@ -150,10 +150,11 @@ static int xdp_umem_account_pages(struct xdp_umem *umem) static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr) { - u32 npgs_rem, chunk_size = mr->chunk_size, headroom = mr->headroom; + u32 chunk_size = mr->chunk_size, headroom = mr->headroom; bool unaligned_chunks = mr->flags & XDP_UMEM_UNALIGNED_CHUNK_FLAG; - u64 npgs, addr = mr->addr, size = mr->len; - unsigned int chunks, chunks_rem; + u64 addr = mr->addr, size = mr->len; + u64 chunks, npgs; + u32 chunks_rem, npgs_rem; 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) return -EINVAL; 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; umem->npgs = (u32)npgs; umem->pgs = NULL; umem->user = NULL;
The number of chunks can overflow u32. Make sure to return -EINVAL on overflow. Fixes: bbff2f321a86 ("xsk: new descriptor addressing scheme") Signed-off-by: Kal Conley <kal.conley@dectris.com> --- net/xdp/xdp_umem.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)