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

[Alexander Lobakin]

From: Kal Conley <kal.conley@xxxxxxxxxxx>
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