On Mon, Apr 10, 2023 at 4:22 AM Lu Wei <[email protected]> wrote:
>
> If an AF_PACKET socket is used to send packets through a L3 mode ipvlan
> and a vnet header is set via setsockopt() with the option name of
> PACKET_VNET_HDR, the value of offset will be nagetive in function
> skb_checksum_help() and trigger the following warning:
>
> WARNING: CPU: 3 PID: 2023 at net/core/dev.c:3262
> skb_checksum_help+0x2dc/0x390
> ......
> Call Trace:
> <TASK>
> ip_do_fragment+0x63d/0xd00
> ip_fragment.constprop.0+0xd2/0x150
> __ip_finish_output+0x154/0x1e0
> ip_finish_output+0x36/0x1b0
> ip_output+0x134/0x240
> ip_local_out+0xba/0xe0
> ipvlan_process_v4_outbound+0x26d/0x2b0
> ipvlan_xmit_mode_l3+0x44b/0x480
> ipvlan_queue_xmit+0xd6/0x1d0
> ipvlan_start_xmit+0x32/0xa0
> dev_hard_start_xmit+0xdf/0x3f0
> packet_snd+0xa7d/0x1130
> packet_sendmsg+0x7b/0xa0
> sock_sendmsg+0x14f/0x160
> __sys_sendto+0x209/0x2e0
> __x64_sys_sendto+0x7d/0x90
>
> The root cause is:
> 1. skb->csum_start is set in packet_snd() according vnet_hdr:
> skb->csum_start = skb_headroom(skb) + (u32)start;
>
> 'start' is the offset from skb->data, and mac header has been
> set at this moment.
>
> 2. when this skb arrives ipvlan_process_outbound(), the mac header
> is unset and skb_pull is called to expand the skb headroom.
>
> 3. In function skb_checksum_help(), the variable offset is calculated
> as:
> offset = skb->csum_start - skb_headroom(skb);
>
> since skb headroom is expanded in step2, offset is nagetive, and it
> is converted to an unsigned integer when compared with skb_headlen
> and trigger the warning.
Not sure why it is negative ? This seems like the real problem...
csum_start is relative to skb->head, regardless of pull operations.
whatever set csum_start to a too small value should be tracked and fixed.
>
> In fact the data to be checksummed should not contain the mac header
> since the mac header is stripped after a packet leaves L2 layer.
> This patch fixes this by adding a check for csum_start to make it
> start after the mac header.
>
> Fixes: 52b5d6f5dcf0 ("net: make skb_partial_csum_set() more robust against overflows")
> Signed-off-by: Lu Wei <[email protected]>
> ---
> net/core/skbuff.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 1a31815104d6..5e24096076fa 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5232,9 +5232,11 @@ bool skb_partial_csum_set(struct sk_buff *skb, u16 start, u16 off)
> u32 csum_end = (u32)start + (u32)off + sizeof(__sum16);
> u32 csum_start = skb_headroom(skb) + (u32)start;
>
> - if (unlikely(csum_start > U16_MAX || csum_end > skb_headlen(skb))) {
> - net_warn_ratelimited("bad partial csum: csum=%u/%u headroom=%u headlen=%u\n",
> - start, off, skb_headroom(skb), skb_headlen(skb));
> + if (unlikely(csum_start > U16_MAX || csum_end > skb_headlen(skb) ||
> + csum_start < skb->network_header)) {
> + net_warn_ratelimited("bad partial csum: csum=%u/%u headroom=%u headlen=%u network_header=%u\n",
> + start, off, skb_headroom(skb),
> + skb_headlen(skb), skb->network_header);
>
I do not understand this patch. You are working around the real bug, right ?
Otherwise we would not have a net_warn_ratelimited() ?
csum_start should actually be at the transport header, so not
considering network header
length seems to call for another bug report when syzbot gets smarter ?
Eric Dumazet wrote:
> On Mon, Apr 10, 2023 at 4:22 AM Lu Wei <[email protected]> wrote:
> >
> > If an AF_PACKET socket is used to send packets through a L3 mode ipvlan
> > and a vnet header is set via setsockopt() with the option name of
> > PACKET_VNET_HDR, the value of offset will be nagetive in function
> > skb_checksum_help() and trigger the following warning:
> >
> > WARNING: CPU: 3 PID: 2023 at net/core/dev.c:3262
> > skb_checksum_help+0x2dc/0x390
> > ......
> > Call Trace:
> > <TASK>
> > ip_do_fragment+0x63d/0xd00
> > ip_fragment.constprop.0+0xd2/0x150
> > __ip_finish_output+0x154/0x1e0
> > ip_finish_output+0x36/0x1b0
> > ip_output+0x134/0x240
> > ip_local_out+0xba/0xe0
> > ipvlan_process_v4_outbound+0x26d/0x2b0
> > ipvlan_xmit_mode_l3+0x44b/0x480
> > ipvlan_queue_xmit+0xd6/0x1d0
> > ipvlan_start_xmit+0x32/0xa0
> > dev_hard_start_xmit+0xdf/0x3f0
> > packet_snd+0xa7d/0x1130
> > packet_sendmsg+0x7b/0xa0
> > sock_sendmsg+0x14f/0x160
> > __sys_sendto+0x209/0x2e0
> > __x64_sys_sendto+0x7d/0x90
> >
> > The root cause is:
> > 1. skb->csum_start is set in packet_snd() according vnet_hdr:
> > skb->csum_start = skb_headroom(skb) + (u32)start;
> >
> > 'start' is the offset from skb->data, and mac header has been
> > set at this moment.
> >
> > 2. when this skb arrives ipvlan_process_outbound(), the mac header
> > is unset and skb_pull is called to expand the skb headroom.
> >
> > 3. In function skb_checksum_help(), the variable offset is calculated
> > as:
> > offset = skb->csum_start - skb_headroom(skb);
> >
> > since skb headroom is expanded in step2, offset is nagetive, and it
> > is converted to an unsigned integer when compared with skb_headlen
> > and trigger the warning.
>
> Not sure why it is negative ? This seems like the real problem...
>
> csum_start is relative to skb->head, regardless of pull operations.
>
> whatever set csum_start to a too small value should be tracked and fixed.
Right. The only way I could see it go negative is if something does
the equivalent of pskb_expand_head with positive nhead, and without
calling skb_headers_offset_update.
Perhaps the cause can be found by instrumenting all the above
functions in the trace to report skb_headroom and csum_start.
And also virtio_net_hdr_to_skb.