netdev - Re: xdp/xsk.c: Possible bug 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: <485c0bfb-8202-4520-92e9-e2bbbf6ac89b@arctic-alpaca.de>
Date: Tue, 9 Jul 2024 13:25:09 +0200
From: Julian Schindel <mail@...tic-alpaca.de>
To: Magnus Karlsson <magnus.karlsson@...il.com>
Cc: bpf@...r.kernel.org, Björn Töpel <bjorn@...nel.org>,
 Magnus Karlsson <magnus.karlsson@...el.com>,
 Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
 Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org
Subject: Re: xdp/xsk.c: Possible bug in xdp_umem_reg version check

On 09.07.24 11:23, Magnus Karlsson wrote:
> On Sun, 7 Jul 2024 at 17:06, Julian Schindel <mail@...tic-alpaca.de> wrote:
>> Hi,
>>
>> [...]
> Thank you for reporting this Julian. This seems to be a bug. If I
> check the value of sizeof(struct xdp_umem_reg_v2), I get 32 bytes too
> on my system, compiling with gcc 11.4. I am not a compiler guy so do
> not know what the rules are for padding structs, but I read the
> following from [0]:
>
> "Pad the entire struct to a multiple of 64-bits if the structure
> contains 64-bit types - the structure size will otherwise differ on
> 32-bit versus 64-bit. Having a different structure size hurts when
> passing arrays of structures to the kernel, or if the kernel checks
> the structure size, which e.g. the drm core does."
>
> I compiled for 64-bits and I believe you did too, but we still get
> this padding. 
Yes, I did also compile for 64-bits. If I understood the resource you
linked correctly, the compiler automatically adding padding to align to
64-bit boundaries is expected for 64-bit platforms:

"[...] 32-bit platforms don’t necessarily align 64-bit values to 64-bit
boundaries, but 64-bit platforms do. So we always need padding to the
natural size to get this right."
> What is sizeof(struct xdp_umem_reg) for you before the
> patch that added tx_metadata_len?
I would expect this to be the same as sizeof(struct xdp_umem_reg_v2)
after the patch. I'm not sure how to check this with different kernel
versions.

Maybe the following code helps show all the sizes
of xdp_umem_reg[_v1/_v2] on my system (compiled with "gcc test.c -o
test" using gcc 14.1.1):

#include <stdio.h>
#include <sys/types.h>

typedef __uint32_t __u32;
typedef __uint64_t __u64;

struct xdp_umem_reg_v1  {
    __u64 addr; /* Start of packet data area */
    __u64 len; /* Length of packet data area */
    __u32 chunk_size;
    __u32 headroom;
};

struct xdp_umem_reg_v2 {
    __u64 addr; /* Start of packet data area */
    __u64 len; /* Length of packet data area */
    __u32 chunk_size;
    __u32 headroom;
    __u32 flags;
};

struct xdp_umem_reg {
    __u64 addr; /* Start of packet data area */
    __u64 len; /* Length of packet data area */
    __u32 chunk_size;
    __u32 headroom;
    __u32 flags;
    __u32 tx_metadata_len;
};

int main() {
    printf("__u32: \t\t\t %lu\n", sizeof(__u32));
    printf("__u64: \t\t\t %lu\n", sizeof(__u64));
    printf("xdp_umem_reg_v1: \t %lu\n", sizeof(struct xdp_umem_reg_v1));
    printf("xdp_umem_reg_v2: \t %lu\n", sizeof(struct xdp_umem_reg_v2));
    printf("xdp_umem_reg: \t\t %lu\n", sizeof(struct xdp_umem_reg));
}

Running "./test" produced this output:

__u32:                   4
__u64:                   8
xdp_umem_reg_v1:         24
xdp_umem_reg_v2:         32
xdp_umem_reg:            32
> [0]: https://www.kernel.org/doc/html/v5.4/ioctl/botching-up-ioctls.html
>> Best regards,
>> Julian