From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>,
virtualization@lists.linux.dev, Spike Du <spiked@nvidia.com>
Subject: Re: [PATCH vhost] virtio_net: update the hdr_len calculation for xmit
Date: Tue, 23 Apr 2024 13:53:49 +0800 [thread overview]
Message-ID: <1713851629.1603248-2-xuanzhuo@linux.alibaba.com> (raw)
In-Reply-To: <20240422163048-mutt-send-email-mst@kernel.org>
On Mon, 22 Apr 2024 16:31:03 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Mar 22, 2024 at 02:08:25PM +0800, Xuan Zhuo wrote:
> > On Fri, 22 Mar 2024 13:57:36 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Fri, Mar 22, 2024 at 1:51 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Fri, 22 Mar 2024 13:04:48 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Fri, Mar 22, 2024 at 10:29 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > >
> > > > > > On Thu, 21 Mar 2024 12:17:54 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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@linux.alibaba.com>
> > > > > > > > Reported-by: Spike Du <spiked@nvidia.com>
> > > > > > > > ---
> > > > > > > > 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.
> > >
> > > Unfortunately not. We can't audit all device implementations.
> > >
> > > > For the driver, we should do as the spec.
> > > > Do you mean that some evil driver pass 0 to device?
> > > > Now we fix that.
> > >
> > > hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb)
> > >
> > > Did you mean hdrlen can't be zero here? I guess it is done by the
> > > DODGY checking in the GSO path?
> >
> > Do you mean tcp_hdrlen(skb) + skb_transport_offset(skb) maybe zero?
> > I think so indeed. I will to check the logic of DODGY.
> >
> > Thanks.
>
> Was any conclusion reached?
No, I am currently more focused on other work.
Thanks.
>
> > >
> > > Then, let's explain it here.
> > >
> > > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > +
> > > > > > > > + } 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.
> > >
> > > Yes, a boolean probably.
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks
> > > >
> > > >
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>
prev parent reply other threads:[~2024-04-23 5:54 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-20 10:20 [PATCH vhost] virtio_net: update the hdr_len calculation for xmit Xuan Zhuo
2024-03-21 4:17 ` Jason Wang
2024-03-22 2:27 ` Xuan Zhuo
2024-03-22 5:04 ` Jason Wang
2024-03-22 5:45 ` Xuan Zhuo
2024-03-22 5:57 ` Jason Wang
2024-03-22 6:08 ` Xuan Zhuo
2024-04-22 20:31 ` Michael S. Tsirkin
2024-04-23 5:53 ` Xuan Zhuo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1713851629.1603248-2-xuanzhuo@linux.alibaba.com \
--to=xuanzhuo@linux.alibaba.com \
--cc=jasowang@redhat.com \
--cc=mst@redhat.com \
--cc=spiked@nvidia.com \
--cc=virtualization@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).