[PATCH v2 0/2] net: prevent infinite loop caused by incorrect proto from virtio_net_hdr_set_proto

[PATCH v2 0/2] net: prevent infinite loop caused by incorrect proto from virtio_net_hdr_set_proto

[Balazs Nemeth]

Here is v2 of the patches that prevent an infinite loop for gso packets
with a protocol from virtio net hdr that doesn't match the protocol in
the packet. Note that packets coming from a device without
header_ops->parse_protocol being implemented will not be caught by
the check in virtio_net_hdr_to_skb, but the infinite loop will still
be prevented by the check in the gso layer.

Balazs Nemeth (2):
  net: check if protocol extracted by virtio_net_hdr_set_proto is
    correct
  net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0

 include/linux/virtio_net.h | 8 +++++++-
 net/mpls/mpls_gso.c        | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

--
2.29.2

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

[Balazs Nemeth]

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;
+			}
 retry:
 			if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
 							      NULL, 0, 0, 0,
-- 
2.29.2

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

[Balazs Nemeth]

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;
 
 	/* Setup inner SKB. */
-- 
2.29.2

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
>

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.

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.

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

[Balazs Nemeth]

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?

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.

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.

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

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).