netdev - Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

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]

Date:   Mon, 1 Jun 2020 22:31:34 -0700
From:   Michael Forney <mforney@...rney.org>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <kernel-team@...com>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants
 used from BPF program side to enums

Hi,

On 2020-03-04, Daniel Borkmann <daniel@...earbox.net> wrote:
> I was about to push the series out, but agree that there may be a risk for
> #ifndefs
> in the BPF C code. If we want to be on safe side, #define FOO FOO would be
> needed.

I did indeed hit some breakage due to this change, but not for the
anticipated reason.

The C standard requires that enumeration constants be representable as
an int, and have type int. While it is a common extension to allow
constants that exceed the limits of int, and this is required
elsewhere in Linux UAPI headers, this is the first case I've
encountered where the constant is not representable as unsigned int
either:

	enum {
		BPF_F_CTXLEN_MASK		= (0xfffffULL << 32),
	};

To see why this can be problematic, consider the following program:

	#include <stdio.h>

	enum {
		A = 1,
		B = 0x80000000,
		C = 1ULL << 32,

		A1 = sizeof(A),
		B1 = sizeof(B),
	};

	enum {
		A2 = sizeof(A),
		B2 = sizeof(B),
	};

	int main(void) {
		printf("sizeof(A) = %d, %d\n", (int)A1, (int)A2);
		printf("sizeof(B) = %d, %d\n", (int)B1, (int)B2);
	}

You might be surprised by the output:

	sizeof(A) = 4, 4
	sizeof(B) = 4, 8

This is because the type of B is different inside and outside the
enum. In my C compiler, I have implemented the extension only for
constants that fit in unsigned int to avoid these confusing semantics.

Since BPF_F_CTXLEN_MASK is the only offending constant, is it possible
to restore its definition as a macro?

Also, I'm not sure if it was considered, but using enums also changes
the signedness of these constants. Many of the previous macro
expressions had type unsigned long long, and now they have type int
(the type of the expression specifying the constant value does not
matter). I could see this causing problems if these constants are used
in expressions involving shifts or implicit conversions.

Thanks for your time,

-Michael