Add variable offset to packet pointer in XDP/TC programs

Add variable offset to packet pointer in XDP/TC programs

[Federico Parola]

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?

Best regards,
Federico Parola

Re: Add variable offset to packet pointer in XDP/TC programs

[Alexei Starovoitov]

On Mon, Nov 8, 2021 at 6:04 AM Federico Parola
<federico.parola@polito.it> 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 += ' ?

Re: Add variable offset to packet pointer in XDP/TC programs

[Alexei Starovoitov]

On Tue, Nov 9, 2021 at 5:47 AM Federico Parola
<federico.parola@polito.it> 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@polito.it> 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 += ' ?
> >

Re: Add variable offset to packet pointer in XDP/TC programs

[Federico Parola]

On 09/11/21 17:13, Alexei Starovoitov wrote:
> On Tue, Nov 9, 2021 at 5:47 AM Federico Parola
> <federico.parola@polito.it> 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?

Definitely not, both my use cases can be solved limiting the size of the 
offset with a mask (I suppose a comparison with an upper limit works as 
well).
Thank you.

>>
>> On 09/11/21 06:29, Alexei Starovoitov wrote:
>>> On Mon, Nov 8, 2021 at 6:04 AM Federico Parola
>>> <federico.parola@polito.it> 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 += ' ?
>>>