On Mon, Nov 8, 2021 at 6:04 AM Federico Parola
<federico.parola@xxxxxxxxx> wrote:
>
> Dear all,
> I found out that every time an offset stored in a 2 (or more) bytes
> variable is added to a packet pointer subsequent checks against packet
> boundaries become ineffective.
> Here is a toy example to test the problem (it doesn't do anything useful):
>
> int test(struct __sk_buff *ctx) {
> void *data = (void *)(long)ctx->data;
> void *data_end = (void *)(long)ctx->data_end;
>
> /* Skipping an amount of bytes stored in __u8 works */
> if (data + sizeof(__u8) > data_end)
> return TC_ACT_OK;
> bpf_trace_printk("Skipping %d bytes", *(__u8 *)data);
> data += *(__u8 *)data;
>
> /* Skipping an amount of bytes stored in __u16 works but... */
> if (data + sizeof(__u16) > data_end)
> return TC_ACT_OK;
> bpf_trace_printk("Skipping %d bytes", *(__u16 *)data);
> data += *(__u16 *)data;
>
> /* ...this check is not effective and packet access is rejected */
> if (data + sizeof(__u8) > data_end)
> return TC_ACT_OK;
> bpf_trace_printk("Next byte is %x", *(__u8 *)data);
>
> return TC_ACT_OK;
> }
>
> My practical use case would be skipping variable-size TLS header
> extensions until I reach the desired one (the length of these options is
> 2 bytes long).
> Another use case can be found here:
> https://lists.iovisor.org/g/iovisor-dev/topic/access_packet_payload_in_tc/86442134
> After I use the bpf_skb_pull_data() I would like to directly jump to the
> part of packet I was working on and avoid re-parsing everything from
> scratch, however if I save the offset in a 2 bytes variable and then add
> it to the packet pointer I'm no longer able to access it (if the offset
> is stored in a 1 byte var everything works).
>
> Is this a verifier bug?
It's because of:
if (dst_reg->umax_value > MAX_PACKET_OFF ||
dst_reg->umax_value + dst_reg->off > MAX_PACKET_OFF)
/* Risk of overflow. For instance, ptr + (1<<63) may be less
* than pkt_end, but that's because it's also less than pkt.
*/
return;
by adding u16 scalar the offset becomes bigger than MAX_PACKET_OFF.
Could you try clamping the value before 'data += ' ?
