Virtualization - Feb 2023 - [patch net-next v2] net: virtio_net: implement exact header length guest feature

Tue, Feb 21, 2023 at 05:12:33PM CET, willemdebruijn.kernel at gmail.com
wrote:
>Jiri Pirko wrote:
>> Tue, Feb 21, 2023 at 04:11:53PM CET, willemdebruijn.kernel at gmail.com
wrote:
>> >Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri at nvidia.com>
>> >> 
>> >> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN
which when
>> >> set implicates that the driver provides the exact size of the
header.
>> >> 
>> >> Quoting the original virtio spec:
>> >> "hdr_len is a hint to the device as to how much of the
header needs to
>> >>  be kept to copy into each packet"
>> >> 
>> >> "a hint" might not be clear for the reader what does
it mean, if it is
>> >> "maybe like that" of "exactly like that".
This feature just makes it
>> >> crystal clear and let the device count on the hdr_len being
filled up
>> >> by the exact length of header.
>> >> 
>> >> Also note the spec already has following note about hdr_len:
>> >> "Due to various bugs in implementations, this field is
not useful
>> >>  as a guarantee of the transport header size."
>> >> 
>> >> Without this feature the device needs to parse the header in
core
>> >> data path handling. Accurate information helps the device to
eliminate
>> >> such header parsing and directly use the hardware accelerators
>> >> for GSO operation.
>> >> 
>> >> virtio_net_hdr_from_skb() fills up hdr_len to
skb_headlen(skb).
>> >> The driver already complies to fill the correct value.
Introduce the
>> >> feature and advertise it.
>> >> 
>> >> Note that virtio spec also includes following note for device
>> >> implementation:
>> >> "Caution should be taken by the implementation so as to
prevent
>> >>  a malicious driver from attacking the device by setting
>> >>  an incorrect hdr_len."
>> >> 
>> >> There is a plan to support this feature in our emulated
device.
>> >> A device of SolidRun offers this feature bit. They claim this
feature
>> >> will save the device a few cycles for every GSO packet.
>> >> 
>> >> Signed-off-by: Jiri Pirko <jiri at nvidia.com>
>> >> ---
>> >> v1->v2:
>> >> - extended patch description
>> >
>> >Is the expectation that in-kernel devices support this feature, and
>> >if so how would it affect them? If I read the spec correctly,
devices
>> 
>> Well, the tap driver actually trusts the hdr_len to be of correct
header
>> size nowadays.
>
>tap_get_user performs basic bounds checking on the length passed.
Sure. It trusts the hdr_len, but it sanitizes the input.

> 
>> 
>> >still need to be careful against malicious drivers, so cannot
assume
>> >much beyond what they do today (i.e., a hint).
>> 
>> Malicious how? There is upper limit of size in tap which is checked.
>> I assume that for hw implementation, that would be the same.
>
>A device cannot blindly trust a hdr_len passed from a driver. We have
>had bugs in the kernel with this before, such as the one fixed in
>commit 57031eb79490 ("packet: round up linear to header len").
>
>> But anyway, this discussion would be rather part of the spec/device
>> patch, don't you think?
>
>I disagree. If it's not much effort to make a commit self-documenting
>that is preferable. And if not, then an explicit reference to an
>authoratitive external reference is preferable over "it is trivial to
>look it up".
Sorry, I don't follow. What exactly do you want me to do?

> 
>> 
>> >
>> >Might be good to point to the definition commit:
>>
>https://github.com/oasis-tcs/virtio-spec/commit/4f1981a1ff46b7aeb801c4c524ff76e93d9ce022
>> 
>> There were couple of fixes to the spec since then, that's why I
didn't
>> include it. It is trivial to look it up in the spec.

Jiri Pirko wrote:
> Tue, Feb 21, 2023 at 05:12:33PM CET, willemdebruijn.kernel at gmail.com
wrote:
> >Jiri Pirko wrote:
> >> Tue, Feb 21, 2023 at 04:11:53PM CET, willemdebruijn.kernel at
gmail.com wrote:
> >> >Jiri Pirko wrote:
> >> >> From: Jiri Pirko <jiri at nvidia.com>
> >> >> 
> >> >> Virtio spec introduced a feature
VIRTIO_NET_F_GUEST_HDRLEN which when
> >> >> set implicates that the driver provides the exact size of
the header.
> >> >> 
> >> >> Quoting the original virtio spec:
> >> >> "hdr_len is a hint to the device as to how much of
the header needs to
> >> >>  be kept to copy into each packet"
> >> >> 
> >> >> "a hint" might not be clear for the reader what
does it mean, if it is
> >> >> "maybe like that" of "exactly like
that". This feature just makes it
> >> >> crystal clear and let the device count on the hdr_len
being filled up
> >> >> by the exact length of header.
> >> >> 
> >> >> Also note the spec already has following note about
hdr_len:
> >> >> "Due to various bugs in implementations, this field
is not useful
> >> >>  as a guarantee of the transport header size."
> >> >> 
> >> >> Without this feature the device needs to parse the header
in core
> >> >> data path handling. Accurate information helps the device
to eliminate
> >> >> such header parsing and directly use the hardware
accelerators
> >> >> for GSO operation.
> >> >> 
> >> >> virtio_net_hdr_from_skb() fills up hdr_len to
skb_headlen(skb).
> >> >> The driver already complies to fill the correct value.
Introduce the
> >> >> feature and advertise it.
> >> >> 
> >> >> Note that virtio spec also includes following note for
device
> >> >> implementation:
> >> >> "Caution should be taken by the implementation so as
to prevent
> >> >>  a malicious driver from attacking the device by setting
> >> >>  an incorrect hdr_len."
> >> >> 
> >> >> There is a plan to support this feature in our emulated
device.
> >> >> A device of SolidRun offers this feature bit. They claim
this feature
> >> >> will save the device a few cycles for every GSO packet.
> >> >> 
> >> >> Signed-off-by: Jiri Pirko <jiri at nvidia.com>
> >> >> ---
> >> >> v1->v2:
> >> >> - extended patch description
> >> >
> >> >Is the expectation that in-kernel devices support this
feature, and
> >> >if so how would it affect them? If I read the spec correctly,
devices
> >> 
> >> Well, the tap driver actually trusts the hdr_len to be of correct
header
> >> size nowadays.
> >
> >tap_get_user performs basic bounds checking on the length passed.
> 
> Sure. It trusts the hdr_len, but it sanitizes the input.
>
> 
> > 
> >> 
> >> >still need to be careful against malicious drivers, so cannot
assume
> >> >much beyond what they do today (i.e., a hint).
> >> 
> >> Malicious how? There is upper limit of size in tap which is
checked.
> >> I assume that for hw implementation, that would be the same.
> >
> >A device cannot blindly trust a hdr_len passed from a driver. We have
> >had bugs in the kernel with this before, such as the one fixed in
> >commit 57031eb79490 ("packet: round up linear to header
len").
> >
> >> But anyway, this discussion would be rather part of the
spec/device
> >> patch, don't you think?
> >
> >I disagree. If it's not much effort to make a commit
self-documenting
> >that is preferable. And if not, then an explicit reference to an
> >authoratitive external reference is preferable over "it is trivial
to
> >look it up".
> 
> Sorry, I don't follow. What exactly do you want me to do?
Either including the link that Michael shared or quoting the relevant
part verbatim in the commit message would help, thanks.

Thinking it over, my main concern is that the prescriptive section in
the spec does not state what to do when the value is clearly garbage,
as we have seen with syzkaller.

Having to sanitize input, by dropping if < ETH_HLEN or > length, to
me means that the device cannot trust the field, as the spec says it
should. 

Sanitization is harder in the kernel, because it has to support all
kinds of link layers, including variable length.

Perhaps that's a discussion for the spec rather than this commit. But
it's a point to clarify as we add support to the code.