Re: 答复: [PATCH net] net: Add check for csum_start in skb_partial_csum_set()

[luwei (O)]

在 2023/4/15 9:47 PM, Willem de Bruijn 写道:
> luwei (O) wrote:
> > 在 2023/4/15 1:20 AM, Willem de Bruijn 写道:
> > > Willem de Bruijn wrote:
> > > > luwei (O) wrote:
> > > > > yes, here is the vnet_hdr:
> > > > >
> > > > >       flags: 3
> > > > >       gso_type: 3
> > > > >       hdr_len: 23
> > > > >       gso_size: 58452
> > > > >       csum_start: 5
> > > > >       csum_offset: 16
> > > > >
> > > > > and the packet:
> > > > >
> > > > > | vnet_hdr | mac header | network header | data ... |
> > > > >
> > > > >     memcpy((void*)0x20000200,
> > > > >            "\x03\x03\x02\x00\x54\xe4\x05\x00\x10\x00\x80\x00\x00\x53\xcc\x9c\x2b"
> > > > >            "\x19\x3b\x00\x00\x00\x89\x4f\x08\x03\x83\x81\x04",
> > > > >            29);
> > > > >     *(uint16_t*)0x200000c0 = 0x11;
> > > > >     *(uint16_t*)0x200000c2 = htobe16(0);
> > > > >     *(uint32_t*)0x200000c4 = r[3];
> > > > >     *(uint16_t*)0x200000c8 = 1;
> > > > >     *(uint8_t*)0x200000ca = 0;
> > > > >     *(uint8_t*)0x200000cb = 6;
> > > > >     memset((void*)0x200000cc, 170, 5);
> > > > >     *(uint8_t*)0x200000d1 = 0;
> > > > >     memset((void*)0x200000d2, 0, 2);
> > > > >     syscall(__NR_sendto, r[1], 0x20000200ul, 0xe45ful, 0ul, 0x200000c0ul, 0x14ul);
> > > > Thanks. So this can happen whenever a packet is injected into the tx
> > > > path with a virtio_net_hdr.
> > > >
> > > > Even if we add bounds checking for the link layer header in pf_packet,
> > > > it can still point to the network header.
> > > >
> > > > If packets are looped to the tx path, skb_pull is common if a packet
> > > > traverses tunnel devices. But csum_start does not directly matter in
> > > > the rx path (CHECKSUM_PARTIAL is just seen as CHECKSUM_UNNECESSARY).
> > > > Until it is forwarded again to the tx path.
> > > >
> > > > So the question is which code calls skb_checksum_start_offset on the
> > > > tx path. Clearly, skb_checksum_help. Also a lot of drivers. Which
> > > > may cast the signed int return value to an unsigned. Even an u8 in
> > > > the first driver I spotted (alx).
> > > >
> > > > skb_postpull_rcsum anticipates a negative return value, as do other
> > > > core functions. So it clearly allowed in certain cases. We cannot
> > > > just bound it.
> > > >
> > > > Summary after a long story: an initial investigation, but I don't have
> > > > a good solution so far. Maybe others have a good suggestiong based on
> > > > this added context.
> > > Specific to skb_checksum_help, it appears that skb_checksum will
> > > work with negative offset just fine.
> >         In this case maybe not, since it checksums from within the mac
> > header, and the mac header
> >
> >          will be stripped when the rx path checks the checksum.
> The header is pulled, but still present. Obviously something bogus gets
> written if the virtio_net_hdr configures csum offload with a bogus
> offset. But as long as the offset is zero or positive from skb->head,
> the checksum helper works as intended.

   OK, Thanks for your reply

>   
> > > Perhaps the only issue is that the WARN_ON_ONCE compares signed to
> > > unsigned, and thus incorrectly interprets a negative offset as
> > >    >= skb_headlen(skb)
> .

--
Best Regards,
Lu Wei