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]

Message-ID: <968526f7-d262-b69e-ca72-f56078158000@intel.com>
Date:   Wed, 8 Mar 2023 14:32:40 +0100
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Kal Cutter Conley <kal.conley@...tris.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

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