On Fri, 22 Mar 2024 13:04:48 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Fri, Mar 22, 2024 at 10:29 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, 21 Mar 2024 12:17:54 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > The virtio spec says:
> > > > If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have
> > > > been negotiated:
> > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated,
> > > > and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver
> > > > MUST set hdr_len to a value equal to the length of the headers,
> > > > including the transport header.
> > > >
> > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated,
> > > > or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set
> > > > hdr_len to a value not less than the length of the headers,
> > > > including the transport header.
> > > >
> > > > So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the
> > > > hdr->hdr_len should be eth header + ip header + tcp/udp header.
> > > >
> > > > But now:
> > > > hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb));
> > > >
> > > > The skb_headlen() returns the linear space of the skb, not the header
> > > > size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has
> > > > not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE.
> > > >
> > > > We do not check the feature of the device. This function is a common
> > > > function used by many places. So we do more stricter work whatever
> > > > the features is negotiated.
> > > >
> > > > For the case skb_is_gso(skb) is false, if none of the
> > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated,
> > > > the spec not define the action of setting hdr_len. Here I set it to
> > > > skb_headlen(). If one of the above features have been negotiated, we
> > > > should set a value not less than the length of "eth header + ip header +
> > > > tcp/udp header". So the skb_headlen() also is a valid value.
> > > >
> > > > For the case skb_is_gso(skb) is true, it implies that one of
> > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been
> > > > negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set
> > > > it to the length of "eth header + ip header + tcp/udp header".
> > > > If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a
> > > > valid value.
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > > > Reported-by: Spike Du <spiked@xxxxxxxxxx>
> > > > ---
> > > > include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++----------
> > > > 1 file changed, 31 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > > > index 4dfa9b69ca8d..51d93f9762d7 100644
> > > > --- a/include/linux/virtio_net.h
> > > > +++ b/include/linux/virtio_net.h
> > > > @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
> > > >
> > > > if (skb_is_gso(skb)) {
> > > > struct skb_shared_info *sinfo = skb_shinfo(skb);
> > > > + u32 hdrlen;
> > > >
> > > > - /* This is a hint as to how much should be linear. */
> > > > - hdr->hdr_len = __cpu_to_virtio16(little_endian,
> > > > - skb_headlen(skb));
> > > > - hdr->gso_size = __cpu_to_virtio16(little_endian,
> > > > - sinfo->gso_size);
> > > > - if (sinfo->gso_type & SKB_GSO_TCPV4)
> > > > + if (sinfo->gso_type & SKB_GSO_TCPV4) {
> > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> > > > - else if (sinfo->gso_type & SKB_GSO_TCPV6)
> > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb);
> > >
> > > So could evil guests give us a 0 hdrlen through this. If yes, it seems
> > > much more dangerous than headlen or we need harden the value as
> > > 9181d6f8a2bb32d158de66a84164fac05e3ddd18 did.
> >
> > Spec says:
> > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, and
> > gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the device MAY use
> > hdr_len as the transport header size. Note: Caution should be taken by
> > the implementation so as to prevent a malicious driver from attacking
> > the device by setting an incorrect hdr_len.
> >
> > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, or
> > gso_type is VIRTIO_NET_HDR_GSO_NONE, the device MAY use hdr_len only as
> > a hint about the transport header size. The device MUST NOT rely on
> > hdr_len to be correct. Note: This is due to various bugs in
> > implementations.
> >
> > The device nerver trust the hdr_len completely.
> > So I think that is safe.
>
> It doesn't, there's no way to audit all the existing implementations of virtio.
>
> For example, have you seen the commit I've pointed to you?
I haven seen it.
But here, this is driver, the device should handen the check.
For the driver, we should do as the spec.
Do you mean that some evil driver pass 0 to device?
Now we fix that.
>
> >
> >
> > >
> > > > +
> > > > + } else if (sinfo->gso_type & SKB_GSO_TCPV6) {
> > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> > > > - else if (sinfo->gso_type & SKB_GSO_UDP_L4)
> > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb);
> > > > +
> > > > + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) {
> > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4;
> > > > - else
> > > > + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb);
> > > > +
> > > > + } else {
> > > > return -EINVAL;
> > > > + }
> > > > +
> > > > + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST
> > > > + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is
> > > > + * negotiated, we MUST set it to the length of "eth header + ip
> > > > + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN
> > > > + * is not negotiated, that still be a valid value.
> > > > + */
> > >
> > > I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much
> > > more safe as we don't want to break legacy devices.
> >
> > I do not get it.
>
> if (GUEST_HDRLEN)
> tell the offset of the payload
> else
> tell the headlen (stick to the old behaviour)
I am ok. Then we need to pass the features of the device to this function.
Thanks
>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > Thanks
> > >
> > >
> >
>
