Re: [PATCH 2/3] Revert "bpf: Fix issue in verifying allow_ptr_leaks"
From: Daniel Borkmann
Date: Thu Sep 14 2023 - 13:25:59 EST
On 9/14/23 6:20 PM, Alexei Starovoitov wrote:
On Wed, Sep 13, 2023 at 5:30 AM Luis Gerhorst <gerhorst@xxxxxxxxx> wrote:
This reverts commit d75e30dddf73449bc2d10bb8e2f1a2c446bc67a2.
To mitigate Spectre v1, the verifier relies on static analysis to deduct
constant pointer bounds, which can then be enforced by rewriting pointer
arithmetic [1] or index masking [2]. This relies on the fact that every
memory region to be accessed has a static upper bound and every date
below that bound is accessible. The verifier can only rewrite pointer
arithmetic or insert masking instructions to mitigate Spectre v1 if a
static upper bound, below of which every access is valid, can be given.
When allowing packet pointer comparisons, this introduces a way for the
program to effectively construct an accessible pointer for which no
static upper bound is known. Intuitively, this is obvious as a packet
might be of any size and therefore 0 is the only statically known upper
bound below of which every date is always accessible (i.e., none).
To clarify, the problem is not that comparing two pointers can be used
for pointer leaks in the same way in that comparing a pointer to a known
scalar can be used for pointer leaks. That is because the "secret"
components of the addresses cancel each other out if the pointers are
into the same region.
With [3] applied, the following malicious BPF program can be loaded into
the kernel without CAP_PERFMON:
r2 = *(u32 *)(r1 + 76) // data
r3 = *(u32 *)(r1 + 80) // data_end
r4 = r2
r4 += 1
if r4 > r3 goto exit
r5 = *(u8 *)(r2 + 0) // speculatively read secret
r5 &= 1 // choose bit to leak
// ... side channel to leak secret bit
exit:
// ...
This is jited to the following amd64 code which still contains the
gadget:
0: endbr64
4: nopl 0x0(%rax,%rax,1)
9: xchg %ax,%ax
b: push %rbp
c: mov %rsp,%rbp
f: endbr64
13: push %rbx
14: mov 0xc8(%rdi),%rsi // data
1b: mov 0x50(%rdi),%rdx // data_end
1f: mov %rsi,%rcx
22: add $0x1,%rcx
26: cmp %rdx,%rcx
29: ja 0x000000000000003f // branch to mispredict
2b: movzbq 0x0(%rsi),%r8 // speculative load of secret
30: and $0x1,%r8 // choose bit to leak
34: xor %ebx,%ebx
36: cmp %rbx,%r8
39: je 0x000000000000003f // branch based on secret
3b: imul $0x61,%r8,%r8 // leak using port contention side channel
3f: xor %eax,%eax
41: pop %rbx
42: leaveq
43: retq
Here I'm using a port contention side channel because storing the secret
to the stack causes the verifier to insert an lfence for unrelated
reasons (SSB mitigation) which would terminate the speculation.
As Daniel already pointed out to me, data_end is even attacker
controlled as one could send many packets of sufficient length to train
the branch prediction into assuming data_end >= data will never be true.
When the attacker then sends a packet with insufficient data, the
Spectre v1 gadget leaks the chosen bit of some value that lies behind
data_end.
The above analysis is correct, but unlike traditional spec_v1
the attacker doesn't control data/data_end.
The attack can send many large packets to train that data + X < data_end
and then send a small packet where CPU will mispredict that branch
and data + X will speculatively read past data_end,
so the attacker can extract a bit past data_end,
but data/data_end themselves cannot be controlled.
So whether this bit 0 or 1 has no bearing.
The attack cannot be repeated for the same location.
The attacker can read one bit 8 times in a row and all of them
will be from different locations in the memory.
Same as reading 8 random bits from 8 random locations.
Hence I don't think this revert is necessary.
I don't believe you can craft an actual exploit.
Your patch 3 says:
/* Speculative access to be prevented. */
+ char secret = *((char *) iph);
+
+ /* Leak the first bit of the secret value that lies behind data_end to a
+ * SMP silbling thread that also executes imul instructions. If the bit
+ * is 1, the silbling will experience a slowdown. */
+ long long x = secret;
+ if (secret & 1) {
+ x *= 97;
+ }
the comment is correct, but speculative access alone is not enough
to leak data.
What you write makes sense, it will probably be hard to craft an exploit.
Where it's a bit more of an unknown to me is whether struct skb_shared_info
could have e.g. destructor_arg rather static (at last the upper addr bits)
so that you would leak out kernel addresses.