[PATCH] xsk: Add missing overflow check in xdp_umem_reg

[PATCH] xsk: Add missing overflow check in xdp_umem_reg

[Kal Conley]

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;
 	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;
-- 
2.39.2

Re: [PATCH] xsk: Add missing overflow check in xdp_umem_reg

[Alexander Lobakin]

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

Re: [PATCH] xsk: Add missing overflow check in xdp_umem_reg

[Kal Cutter Conley]

> 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

[PATCH] xsk: Add missing overflow check in xdp_umem_reg

[Kal Conley]

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..02207e852d79 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;
 	bool unaligned_chunks = mr->flags & XDP_UMEM_UNALIGNED_CHUNK_FLAG;
-	u64 npgs, addr = mr->addr, size = mr->len;
-	unsigned int chunks, chunks_rem;
+	u32 chunk_size = mr->chunk_size, headroom = mr->headroom;
+	u64 addr = mr->addr, size = mr->len;
+	u32 chunks_rem, npgs_rem;
+	u64 chunks, npgs;
 	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 || chunks > U32_MAX)
 		return -EINVAL;
 
 	if (!unaligned_chunks && chunks_rem)
@@ -202,7 +203,7 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
 	umem->headroom = headroom;
 	umem->chunk_size = chunk_size;
 	umem->chunks = chunks;
-	umem->npgs = (u32)npgs;
+	umem->npgs = npgs;
 	umem->pgs = NULL;
 	umem->user = NULL;
 	umem->flags = mr->flags;
-- 
2.39.2

Re: [PATCH] xsk: Add missing overflow check in xdp_umem_reg

[Alexander Lobakin]

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

Re: [PATCH] xsk: Add missing overflow check in xdp_umem_reg

[Alexander Lobakin]

From: Kal Conley <kal.conley@dectris.com>
Date: Wed,  8 Mar 2023 11:51:30 +0100

> [PATCH] xsk: Add missing overflow check in xdp_umem_reg

You need to mark it properly. It must've been

[PATCH bpf v2] xsk: Add missing overflow check in xdp_umem_reg

instead.

> The number of chunks can overflow u32. Make sure to return -EINVAL on
> overflow.

I'd mention here that cast removal, so that reviewers wouldn't ask why
you did this.

> 
> 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..02207e852d79 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;
>  	bool unaligned_chunks = mr->flags & XDP_UMEM_UNALIGNED_CHUNK_FLAG;
> -	u64 npgs, addr = mr->addr, size = mr->len;
> -	unsigned int chunks, chunks_rem;
> +	u32 chunk_size = mr->chunk_size, headroom = mr->headroom;
> +	u64 addr = mr->addr, size = mr->len;
> +	u32 chunks_rem, npgs_rem;
> +	u64 chunks, npgs;
>  	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 || chunks > U32_MAX)
>  		return -EINVAL;
>  
>  	if (!unaligned_chunks && chunks_rem)
> @@ -202,7 +203,7 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
>  	umem->headroom = headroom;
>  	umem->chunk_size = chunk_size;
>  	umem->chunks = chunks;
> -	umem->npgs = (u32)npgs;
> +	umem->npgs = npgs;
>  	umem->pgs = NULL;
>  	umem->user = NULL;
>  	umem->flags = mr->flags;

The code is fine to me.
Please resubmit with the fixed subject and expanded commit message.
I'd also prefer that you sent v3 as a separate mail, *not* as a reply to
this thread.

Thanks,
Olek

Re: [PATCH] xsk: Add missing overflow check in xdp_umem_reg

[Kal Cutter Conley]

> The code is fine to me.
> Please resubmit with the fixed subject and expanded commit message.
> I'd also prefer that you sent v3 as a separate mail, *not* as a reply to
> this thread.

Done. I used "bpf" in the subject as you suggested, however I am a bit
confused by this. Should changes under net/xdp generally use "bpf" in
the subject?

Thanks,
Kal

Re: [PATCH] xsk: Add missing overflow check in xdp_umem_reg

[Alexander Lobakin]

From: Kal Conley <kal.conley@dectris.com>
Date: Wed, 8 Mar 2023 19:49:29 +0100

>> The code is fine to me.
>> Please resubmit with the fixed subject and expanded commit message.
>> I'd also prefer that you sent v3 as a separate mail, *not* as a reply to
>> this thread.
> 
> Done. I used "bpf" in the subject as you suggested, however I am a bit
> confused by this. Should changes under net/xdp generally use "bpf" in
> the subject?

"bpf" when it's a fix (better to have some real repro, otherwise purely
hypothetical fix can be considered a bpf-next material), "bpf-next" when
it's an improvement / new stuff etc.

Also please don't forget to manually add all the folks who reviewed your
previous versions / were participating in the threads for previous
versions, otherwise they can miss the fact that you posted a new revision.

> 
> Thanks,
> Kal

Thanks,
Olek