Re: [PATCH v2 1/2] net: check if protocol extracted by virtio_net_hdr_set_proto is correct

Re: [PATCH v2 1/2] net: check if protocol extracted by virtio_net_hdr_set_proto is correct

[Willem de Bruijn]

On Mon, Mar 8, 2021 at 5:32 AM Balazs Nemeth <bnemeth@redhat.com> wrote:
>
> For gso packets, virtio_net_hdr_set_proto sets the protocol (if it isn't
> set) based on the type in the virtio net hdr, but the skb could contain
> anything since it could come from packet_snd through a raw socket. If
> there is a mismatch between what virtio_net_hdr_set_proto sets and
> the actual protocol, then the skb could be handled incorrectly later
> on.
>
> An example where this poses an issue is with the subsequent call to
> skb_flow_dissect_flow_keys_basic which relies on skb->protocol being set
> correctly. A specially crafted packet could fool
> skb_flow_dissect_flow_keys_basic preventing EINVAL to be returned.
>
> Avoid blindly trusting the information provided by the virtio net header
> by checking that the protocol in the packet actually matches the
> protocol set by virtio_net_hdr_set_proto. Note that since the protocol
> is only checked if skb->dev implements header_ops->parse_protocol,
> packets from devices without the implementation are not checked at this
> stage.
>
> Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets")
> Signed-off-by: Balazs Nemeth <bnemeth@redhat.com>

Going forward, please mark your the patch as targeting the net tree
using [PATCH net]

> ---
>  include/linux/virtio_net.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index e8a924eeea3d..6c478eee0452 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -79,8 +79,14 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>                 if (gso_type && skb->network_header) {
>                         struct flow_keys_basic keys;
>
> -                       if (!skb->protocol)
> +                       if (!skb->protocol) {
> +                               const struct ethhdr *eth = skb_eth_hdr(skb);

eth is no longer used.

> +                               __be16 etype = dev_parse_header_protocol(skb);

nit: customary to call this protocol. etype, I guess short for
EtherType, makes sense, but is not commonly used in the kernel.

> +
>                                 virtio_net_hdr_set_proto(skb, hdr);
> +                               if (etype && etype != skb->protocol)
> +                                       return -EINVAL;
> +                       }
>  retry:
>                         if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
>                                                               NULL, 0, 0, 0,
> --
> 2.29.2
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/2] net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0

[Willem de Bruijn]

On Mon, Mar 8, 2021 at 5:32 AM Balazs Nemeth <bnemeth@redhat.com> wrote:
>
> A packet with skb_inner_network_header(skb) == skb_network_header(skb)
> and ETH_P_MPLS_UC will prevent mpls_gso_segment from pulling any headers
> from the packet. Subsequently, the call to skb_mac_gso_segment will
> again call mpls_gso_segment with the same packet leading to an infinite
> loop.
>
> Signed-off-by: Balazs Nemeth <bnemeth@redhat.com>
> ---
>  net/mpls/mpls_gso.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
> index b1690149b6fa..cc1b6457fc93 100644
> --- a/net/mpls/mpls_gso.c
> +++ b/net/mpls/mpls_gso.c
> @@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,
>
>         skb_reset_network_header(skb);
>         mpls_hlen = skb_inner_network_header(skb) - skb_network_header(skb);
> -       if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
> +       if (unlikely(!mpls_hlen || !pskb_may_pull(skb, mpls_hlen)))
>                 goto out;

Good cathc. Besides length zero, this can be more strict: a label is
4B, so mpls_hlen needs to be >= 4B.

Perhaps even aligned to 4B, too, but not if there may be other encap on top.

Unfortunately there is no struct or type definition that we can use a
sizeof instead of open coding the raw constant.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/2] net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0

[David Ahern]

On 3/8/21 9:07 AM, Willem de Bruijn wrote:
>> diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
>> index b1690149b6fa..cc1b6457fc93 100644
>> --- a/net/mpls/mpls_gso.c
>> +++ b/net/mpls/mpls_gso.c
>> @@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,
>>
>>         skb_reset_network_header(skb);
>>         mpls_hlen = skb_inner_network_header(skb) - skb_network_header(skb);
>> -       if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
>> +       if (unlikely(!mpls_hlen || !pskb_may_pull(skb, mpls_hlen)))
>>                 goto out;
> 
> Good cathc. Besides length zero, this can be more strict: a label is
> 4B, so mpls_hlen needs to be >= 4B.
> 
> Perhaps even aligned to 4B, too, but not if there may be other encap on top.
> 
> Unfortunately there is no struct or type definition that we can use a
> sizeof instead of open coding the raw constant.
> 

MPLS_HLEN can be used here.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/2] net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0

[David Ahern]

On 3/8/21 9:26 AM, Balazs Nemeth wrote:
> On Mon, 2021-03-08 at 09:17 -0700, David Ahern wrote:
>> On 3/8/21 9:07 AM, Willem de Bruijn wrote:
>>>> diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
>>>> index b1690149b6fa..cc1b6457fc93 100644
>>>> --- a/net/mpls/mpls_gso.c
>>>> +++ b/net/mpls/mpls_gso.c
>>>> @@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(struct
>>>> sk_buff *skb,
>>>>
>>>>         skb_reset_network_header(skb);
>>>>         mpls_hlen = skb_inner_network_header(skb) -
>>>> skb_network_header(skb);
>>>> -       if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
>>>> +       if (unlikely(!mpls_hlen || !pskb_may_pull(skb,
>>>> mpls_hlen)))
>>>>                 goto out;
>>>
>>> Good cathc. Besides length zero, this can be more strict: a label
>>> is
>>> 4B, so mpls_hlen needs to be >= 4B.
>>>
>>> Perhaps even aligned to 4B, too, but not if there may be other
>>> encap on top.
>>>
>>> Unfortunately there is no struct or type definition that we can use
>>> a
>>> sizeof instead of open coding the raw constant.
>>>
>>
>> MPLS_HLEN can be used here.
>>
> 
> What about sizeof(struct mpls_label), like in net/ipv4/tunnel4.c?
> 

I was thinking MPLS_HLEN because of its consistent use with skb
manipulations. net/mpls code uses mpls_shim_hdr over mpls_label.

Looks like the MPLS code could use some cleanups to make this consistent.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/2] net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0

[Willem de Bruijn]

On Mon, Mar 8, 2021 at 11:43 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 3/8/21 9:26 AM, Balazs Nemeth wrote:
> > On Mon, 2021-03-08 at 09:17 -0700, David Ahern wrote:
> >> On 3/8/21 9:07 AM, Willem de Bruijn wrote:
> >>>> diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
> >>>> index b1690149b6fa..cc1b6457fc93 100644
> >>>> --- a/net/mpls/mpls_gso.c
> >>>> +++ b/net/mpls/mpls_gso.c
> >>>> @@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(struct
> >>>> sk_buff *skb,
> >>>>
> >>>>         skb_reset_network_header(skb);
> >>>>         mpls_hlen = skb_inner_network_header(skb) -
> >>>> skb_network_header(skb);
> >>>> -       if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
> >>>> +       if (unlikely(!mpls_hlen || !pskb_may_pull(skb,
> >>>> mpls_hlen)))
> >>>>                 goto out;
> >>>
> >>> Good cathc. Besides length zero, this can be more strict: a label
> >>> is
> >>> 4B, so mpls_hlen needs to be >= 4B.
> >>>
> >>> Perhaps even aligned to 4B, too, but not if there may be other
> >>> encap on top.

On second thought, since mpls_gso_segment pulls all these headers, it
is correct to require it to be a multiple of MPLS_HLEN.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/2] net: check if protocol extracted by virtio_net_hdr_set_proto is correct

[Michael S. Tsirkin]

On Mon, Mar 08, 2021 at 11:31:25AM +0100, Balazs Nemeth wrote:
> For gso packets, virtio_net_hdr_set_proto sets the protocol (if it isn't
> set) based on the type in the virtio net hdr, but the skb could contain
> anything since it could come from packet_snd through a raw socket. If
> there is a mismatch between what virtio_net_hdr_set_proto sets and
> the actual protocol, then the skb could be handled incorrectly later
> on.
> 
> An example where this poses an issue is with the subsequent call to
> skb_flow_dissect_flow_keys_basic which relies on skb->protocol being set
> correctly. A specially crafted packet could fool
> skb_flow_dissect_flow_keys_basic preventing EINVAL to be returned.
> 
> Avoid blindly trusting the information provided by the virtio net header
> by checking that the protocol in the packet actually matches the
> protocol set by virtio_net_hdr_set_proto. Note that since the protocol
> is only checked if skb->dev implements header_ops->parse_protocol,
> packets from devices without the implementation are not checked at this
> stage.
> 
> Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets")
> Signed-off-by: Balazs Nemeth <bnemeth@redhat.com>
> ---
>  include/linux/virtio_net.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index e8a924eeea3d..6c478eee0452 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -79,8 +79,14 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>  		if (gso_type && skb->network_header) {
>  			struct flow_keys_basic keys;
>  
> -			if (!skb->protocol)
> +			if (!skb->protocol) {
> +				const struct ethhdr *eth = skb_eth_hdr(skb);
> +				__be16 etype = dev_parse_header_protocol(skb);
> +
>  				virtio_net_hdr_set_proto(skb, hdr);
> +				if (etype && etype != skb->protocol)
> +					return -EINVAL;
> +			}


Well the protocol in the header is an attempt at an optimization to
remove need to parse the packet ... any data on whether this
affecs performance?

>  retry:
>  			if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
>  							      NULL, 0, 0, 0,
> -- 
> 2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/2] net: check if protocol extracted by virtio_net_hdr_set_proto is correct

[Willem de Bruijn]

On Tue, Mar 9, 2021 at 6:26 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Mar 08, 2021 at 11:31:25AM +0100, Balazs Nemeth wrote:
> > For gso packets, virtio_net_hdr_set_proto sets the protocol (if it isn't
> > set) based on the type in the virtio net hdr, but the skb could contain
> > anything since it could come from packet_snd through a raw socket. If
> > there is a mismatch between what virtio_net_hdr_set_proto sets and
> > the actual protocol, then the skb could be handled incorrectly later
> > on.
> >
> > An example where this poses an issue is with the subsequent call to
> > skb_flow_dissect_flow_keys_basic which relies on skb->protocol being set
> > correctly. A specially crafted packet could fool
> > skb_flow_dissect_flow_keys_basic preventing EINVAL to be returned.
> >
> > Avoid blindly trusting the information provided by the virtio net header
> > by checking that the protocol in the packet actually matches the
> > protocol set by virtio_net_hdr_set_proto. Note that since the protocol
> > is only checked if skb->dev implements header_ops->parse_protocol,
> > packets from devices without the implementation are not checked at this
> > stage.
> >
> > Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets")
> > Signed-off-by: Balazs Nemeth <bnemeth@redhat.com>
> > ---
> >  include/linux/virtio_net.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index e8a924eeea3d..6c478eee0452 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -79,8 +79,14 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> >               if (gso_type && skb->network_header) {
> >                       struct flow_keys_basic keys;
> >
> > -                     if (!skb->protocol)
> > +                     if (!skb->protocol) {
> > +                             const struct ethhdr *eth = skb_eth_hdr(skb);
> > +                             __be16 etype = dev_parse_header_protocol(skb);
> > +
> >                               virtio_net_hdr_set_proto(skb, hdr);
> > +                             if (etype && etype != skb->protocol)
> > +                                     return -EINVAL;
> > +                     }
>
>
> Well the protocol in the header is an attempt at an optimization to
> remove need to parse the packet ... any data on whether this
> affecs performance?

This adds a branch and reading a cacheline that is inevitably read not
much later. It shouldn't be significant.

And this branch is only taken if skb->protocol is not set. So the cost
can easily be avoided by passing the information.

But you raise a good point, because TUNTAP does set it, but only after
the call to virtio_net_hdr_to_skb.

That should perhaps be inverted (in a separate net-next patch).
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization