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: <CAJ8uoz0w9RhAk2v4G-FSzjOCqitCPhEXOC6c_PcOFr7PxTjbWg@mail.gmail.com>
Date: Tue, 9 Jul 2024 11:23:07 +0200
From: Magnus Karlsson <magnus.karlsson@...il.com>
To: Julian Schindel <mail@...tic-alpaca.de>
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 Sun, 7 Jul 2024 at 17:06, Julian Schindel <mail@...tic-alpaca.de> wrote:
>
> Hi,
>
> I hope this is the correct way to ask about this issue, I haven't used
> the kernel mailing list before.
>
> Between different compilations of an AF_XDP project, I encountered
> "random" EINVAL errors when calling setsockopt XDP_UMEM_REG with the
> same parameter.
>
> I think this might be caused by this patch:
> https://lore.kernel.org/all/20231127190319.1190813-2-sdf@google.com/
> It added "tx_metadata_len" to the "xdp_umem_reg" struct.
> In the  "xsk_setsockopt" code in xdp/xsk.c, the provided "optlen" is
> checked against the length of "xdp_umem_reg_v2" and "xdp_umem_reg" to
> check which version of "xdp_umem_reg", the user supplied.
>
> At least on my machine (x86_64, Fedora 40, 6.9.7), these two structs
> have the same size (32 bytes) due to the compiler adding padding to
> "xdp_umem_reg_v2". This means if the user supplies "xdp_umem_reg_v2", it
> is falsely treated as "xdp_umem_reg".
>
> I'm not sure whether there is some implicit struct packing happening or
> whether this is indeed a bug.

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. What is sizeof(struct xdp_umem_reg) for you before the
patch that added tx_metadata_len?

[0]: https://www.kernel.org/doc/html/v5.4/ioctl/botching-up-ioctls.html

> Best regards,
> Julian
>
>