Message-ID: <636d3434-d47a-4cd4-b3ba-7f7254317b64@sylv.io>
Date:   Mon, 4 Sep 2023 12:08:52 +0200
From:   Marcello Sylverster Bauer <sylv@...v.io>
To:     Eyal Birger <eyal.birger@...il.com>
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] xfrm: Use skb_mac_header_was_set() to check for
 MAC header presence

Hi Eval,

On 9/2/23 20:39, Eyal Birger wrote:
> Hi Marcello,
> 
> On Fri, Sep 1, 2023 at 7:15 PM Marcello Sylvester Bauer
> <email@....codeaurora.org> wrote:
>>
>> From: Marcello Sylvester Bauer <sylv@...v.io>
>>
>> Replace skb->mac_len with skb_mac_header_was_set() in
>> xfrm4_remove_tunnel_encap() and xfrm6_remove_tunnel_encap() to detect
>> the presence of a MAC header. This change prevents a kernel page fault
>> that could occur when a MAC address is added to an L3 interface using
>> xfrm.
>>
>> Signed-off-by: Marcello Sylvester Bauer <sylv@...v.io>
>> ---
>>   net/xfrm/xfrm_input.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
>> index d5ee96789d4b..afa1f1052065 100644
>> --- a/net/xfrm/xfrm_input.c
>> +++ b/net/xfrm/xfrm_input.c
>> @@ -250,7 +250,7 @@ static int xfrm4_remove_tunnel_encap(struct xfrm_state *x, struct sk_buff *skb)
>>
>>          skb_reset_network_header(skb);
>>          skb_mac_header_rebuild(skb);
>> -       if (skb->mac_len)
>> +       if (skb_mac_header_was_set(skb))
>>                  eth_hdr(skb)->h_proto = skb->protocol;
>>
>>          err = 0;
>> @@ -287,7 +287,7 @@ static int xfrm6_remove_tunnel_encap(struct xfrm_state *x, struct sk_buff *skb)
>>
>>          skb_reset_network_header(skb);
>>          skb_mac_header_rebuild(skb);
>> -       if (skb->mac_len)
>> +       if (skb_mac_header_was_set(skb))
>>                  eth_hdr(skb)->h_proto = skb->protocol;
> 
> I checked xfrm tunnels over both GRE and IPIP, and in both cases when reaching
> this code the skb->mac_len was 0, whereas skb_mac_header_was_set()
> was 1.
> 
> So in both cases this suggested patch would make this condition true and
> write to an Ethernet header location on these L3 devices.

Oh, so my first guess was right and we need to check for both.

We were able to get socket buffers where the mac_len was non-zero, but 
the mac_header was not set. Not sure if this is a bug from somewhere 
else, but this causes an "unable to handle page fault for address XXX" 
error.

I have attached a decoded crash log to this email using kernel version 
6.1.38.

As you can see, the line net/xfrm/xfrm_input.c:290, which contains the 
following, causes the error:
```
eth_hdr(skb)->h_proto = skb->protocol;
```

IIUC RAX contains mac_header and is set to 0xffff, meaning it is not 
set. This causes skb_mac_header to reference a wrong address for the 
header, resulting in the page fault.

> 
> Can you please share your reproduction scenario for this case?

We have a special setup where a WWAN interface is forwarded to a QEMU VM 
as a virtual network interface. Here is the relevant information based 
on the internal bug report that addresses this issue:

---

Kernel version: 6.1.38

Problem occurs with quectel em120r-gl and Intel 5000 5G modems.
Both modems do not support bridge mode, i.e. no L2 but only L3 ipraw 
communication. Specifically, the interface has no MAC address.

Our application uses by default the MAC address of the physical adapter 
for the virtual adapter, so e.g. host entries in dnsmasq are created in 
the direction of windows VM.

Modify net_hotplug in udev: Assign fake MAC address to wwan interface 
(actually writes only the MAC to a file, which is passed to the VM as 
MAC in the hotplug case)

# mac address required for hotplug -> end
#ATTRS{address}=="", GOTO="tvpnc_network_end"

ACTION=="add", ATTRS{address}=="", \
     NAME=="en*|wl*|ww*", \
     RUN+="/usr/share/scripts/net_hotplug.sh -a -i $name -m 
aa:bb:cc:dd:ee:ff"

ACTION=="add", ATTRS{address}!="", \
     NAME=="en*|wl*|ww*", \
     RUN+="/usr/share/scripts/net_hotplug.sh -a -i $name -m $attr{address}"

When Windows establishes a cellular connection and connects to a 
strongSwan VPN, the host Linux kernel crashes and immediately restarts.

---

The custom QEMU hotplug script net_hotplug.sh, which adds the virtual 
network interface to the running VM and binds it to the VM using the 
real or the "fake" Mac address.

I got feedback that adding "skb_mac_header_was_set(skb)" to the 
condition fixed the problem.

The 2nd version of this patch will check for both bot, mac_len and if 
the header was set.

Thanks
Marcello

> 
> Thanks,
> Eyal.
> 
>>
>>          err = 0;
>> --
>> 2.42.0
>>
>>
View attachment "decoded_crash.log" of type "text/x-log" (10007 bytes)

Powered by blists - more mailing lists