On Tue, Nov 9, 2021 at 5:47 AM Federico Parola
<federico.parola@xxxxxxxxx> wrote:
>
> Thanks for your answer.
Please do not top post and don't drop mailing list.
> If I perform something like:
>
> *(__u16 *)data &= 0xefff;
> data += *(__u16 *)data;
>
> To limit the max value of my offset the program is accepted, is this
> what you mean with "clamping"?
kinda. I don't think you meant to mangle the packet in the above.
> So packet pointers are stored on 16 bits? And every time we add an
> offset we must guarantee not to overflow these size?
no. the pointers are 64-bit, but there could be additional alu ops
on them. So MAX_PACKET_OFF was picked as the practical limit.
Do you have a real life use case where you need to add full u16?
>
> On 09/11/21 06:29, Alexei Starovoitov wrote:
> > 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 += ' ?
> >
