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:   Tue, 2 Jun 2020 12:17:03 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Michael Forney <mforney@...rney.org>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        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

On Mon, Jun 01, 2020 at 10:31:34PM -0700, Michael Forney wrote:
> 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?

It's possible, but I'm not sure what it will fix.
Your example is a bit misleading, since it's talking about B
which doesn't have type specifier, whereas enums in bpf.h have ULL
suffix where necessary.
And the one you pointed out BPF_F_CTXLEN_MASK has sizeof == 8 in all cases.

Also when B is properly annotated like 0x80000000ULL it will have size 8
as well.

#include <stdio.h>

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

        A1 = sizeof(A),
        B1 = sizeof(B),
        C1 = sizeof(C),
        D1 = sizeof(D),
};

enum {
        A2 = sizeof(A),
        B2 = sizeof(B),
        C2 = sizeof(C),
        D2 = sizeof(D),
};

int main(void) {
        printf("sizeof(A) = %d, %d\n", (int)A1, (int)A2);
        printf("sizeof(B) = %d, %d\n", (int)B1, (int)B2);
        printf("sizeof(C) = %d, %d\n", (int)C1, (int)C2);
        printf("sizeof(D) = %d, %d\n", (int)D1, (int)D2);
}

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

So the problem is only with non-annotated enums that are mixed
in a enum with some values <32bit and others >32 bit.
bpf.h has only one such enum:
enum {
        BPF_F_INDEX_MASK                = 0xffffffffULL,
        BPF_F_CURRENT_CPU               = BPF_F_INDEX_MASK,
        BPF_F_CTXLEN_MASK               = (0xfffffULL << 32),
};

and all values are annotated with ULL.
So I really don't see a problem.

> 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.

It would have been if the enums were not annotated. But that's not the case.