fix(driver/bpf): bpf_parse_readv_writev_bufs avoid overflow while reading readv/writev iovec struct

[fntlnz]

Reading the buffer data from the iovec struct requires to go take a specific area in our buffer to perform the read into the cpu data->buf
map.

New kernels implement the __check_mem_access which takes care
of checking if the accessed memory is valid or not in the current
context.

In our case since our scratch buffer is split in a half with two null
terminators, we only need to read the area until the null terminator
happens or the verifier will alert.

Here's the verifier check 0 for this case:

    bool size_ok = size > 0 || (size == 0 && zero_size_allowed);
    struct bpf_reg_state *reg;

    if (off >= 0 && size_ok && (u64)off + size <= mem_size)
	    return 0;

Refs #1658

Co-Authored-By: Leonardo Di Donato leodidonato@gmail.com
Signed-off-by: Lorenzo Fontana lo@linux.com

[nathan-b]

I was having some trouble understanding exactly what effect this change might have, so I wrote a scratch program. I'm mapping the difference in read offset between the old code and the new code:

0:
 New: 0
 Old: 0
1:
 New: 1
 Old: 0
2:
 New: 2
 Old: 2
1000:
 New: 1000
 Old: 1000
2321:
 New: 2321
 Old: 2320
131069: (= SCRATCH_SIZE_HALF - 2)
 New: 131069
 Old: 131068
131070: (= SCRATCH_SIZE_HALF - 1)
 New: 131070
 Old: 131070
131071: (= SCRATCH_SIZE_HALF)
 New: 131071
 Old: 131070
131072: (= SCRATCH_SIZE_HALF + 1)
 New: 0
 Old: 0

My concern is that when off == 1, the old code will read from offset 0 but the new code will read from offset 1, which could have very real repercussions on the data.

And of course, it's not just 1. Any value of off which has the least significant bit set will now be masked differently.

Maybe I just don't understand the intent of this fix, but I'm worried it will cause the probe to start emitting incorrect data!

[fntlnz]

The key to understanding the change here is that data->buf is the destination map where the data from the kernel is copied. So the data buffer is not cut, we are just making sure that the buffer size does not exceed the scratch location when writing into it.

In any case, even if the source buffer was bigger, our architecture does not support reading more than SCRATCH_SIZE_HALF and, even if it could sinsp and scap have hard limits on what is brought into userspace and ultimately at the presentation layer.

[fntlnz]

Moreover, a bug in the offsets here would only affect readv and writev syscalls.

[gnosek]

Parentheses? Is this expression (off & SCRATCH_SIZE_HALF) - 1 or off & (SCRATCH_SIZE_HALF - 1)?

[fntlnz]

- has precedence over & and I confirm that the generated bpf bytecode is correct.

[nathan-b]

Based on what you wrote above, is it possible that the intent of the code is (off & SCRATCH_SIZE_HALF) - 1?

[leodido]

Hold until we test this on older verifiers (ie., older kernels).

[fntlnz]

This can be closed, I will post updates on this fix later, debugged the kernel a bit more and understood why this is happening. Stay tuned.