fix(driver/bpf): bpf_addr_to_kernel takes an unsigned length (less than BPF_MAX_VAR_SIZE) #1730
Conversation
BPF_MAX_VAR_SIZE) The `bpf_syscall_get_argument` function returns an `unsigned long` that was assigned to an `int` and passed to `bpf_addr_to_kernel` (`ulen` variable). We need to enusre the value is always unsigned (thus removing the inner check for it being greater than 0). We also need to ensure it is not bigger than `1ULL << 29` (ie., `BPF_MAX_VAR_SIZE`). Otherwise the verifier gives un an "unbounded memory access" error. This happens for Linux kernels containing [this patch](https://lore.kernel.org/bpf/20190614072557.196239-10-ast@kernel.org/). Co-authored-by: Lorenzo Fontana <lo@linux.com> Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
| struct sockaddr *kaddr) | ||
| { | ||
| if (ulen < 0 || ulen > sizeof(struct sockaddr_storage)) | ||
| if (ulen > BPF_MAX_VAR_SIZE) |
There was a problem hiding this comment.
This seems to increase the valid range of ulen. Previously the function would return -EINVAL if ulen > sizeof(struct sockaddr_storage), but on my machine that sizeof operation resolves to 128, which is far, far less than BPF_MAX_VAR_SIZE. Is this intended?
There was a problem hiding this comment.
Looks like we're doing ulen & 0xff (more or less) below. I feel that we should return an error whenever ulen > 0xff (otherwise the caller can request e.g. 0x1ff bytes--the value comes straight from userspace--while we copy 0xff and return success).
The original check against sizeof(struct sockaddr_storage) feels more correct.
Also, not sure what exactly BPF_FORBIDS_ZERO_ACCESS implies but if my guess (length must be > 0) is correct, both the #ifdef and the & 0xff (if we revert the size check) is unnecessary (ulen will always be > 0 after line 224 and < 0xff after original line 218).
Though I expect both are there to keep the verifier happy?
There was a problem hiding this comment.
That check seems like a leftover from move_addr_to_kernel kernel function.
Anyways, we read at maximum ulen & 0xff every time (see bpf_probe_read below).
After some intensive and day-long testing, we (@fntlnz and I) have not been able to break it.
There was a problem hiding this comment.
@gnosek The verifier rejects a check against sizeof(struct sockaddr_storage).
There was a problem hiding this comment.
Anyway the bug here was about the int (now unsigned long) parameter and the missing boundary check.
Both were required by the verifier (giving two different errors on each of them). Thus, this is the scope of this PR.
In case we'd like to optimize the behaviour of the mask and the presence (or not) of ifdefs, we should approach those in another PR, imho.
As per our testing, they do not interfere with the correct functioning of the probe.
There was a problem hiding this comment.
So if you replace BPF_MAX_VAR_SIZE with sizeof(struct sockaddr_storage), the verifier fails?
That is bizarre. That seems like a bug in the verifier.
There was a problem hiding this comment.
Yes, we tried in many many ways to retain that check too, but the verifier rejects it.
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
|
Hold until we test this on older verifiers (ie., older kernels). |
The
bpf_syscall_get_argumentfunction returns anunsigned longthatwas assigned to an
intand passed to thebpf_addr_to_kernelfunction (ulenvariable).
We need to ensure the value is always unsigned (thus removing the inner
check
> 0).We also need to ensure it is not bigger than
1ULL << 29(ie.,BPF_MAX_VAR_SIZE).Otherwise, the verifier gives un an "unbounded memory access" error.
This happens for Linux kernels (eg., >= 5.8) containing this
patch.
Refs #1658
Co-authored-by: Lorenzo Fontana lo@linux.com
Signed-off-by: Leonardo Di Donato leodidonato@gmail.com