Re: [PATCH] net: check for dev pointer being NULL in dev_hard_header() to avoid GPF

[George Kennedy <george.kennedy@oracle.com>]

Hi Eric,

More info...

On 12/1/2022 11:11 PM, Eric Dumazet wrote:
> On Thu, Dec 1, 2022 at 9:44 PM George Kennedy <george.kennedy@oracle.com> wrote:
>>
>>
>> On 12/1/2022 2:25 PM, Eric Dumazet wrote:
>>> On Thu, Dec 1, 2022 at 2:16 PM Pavan Chebbi <pavan.chebbi@broadcom.com> wrote:
>>>> On Wed, Nov 30, 2022 at 7:43 PM George Kennedy
>>>> <george.kennedy@oracle.com> wrote:
>>>>> The dev pointer can be NULL in dev_hard_header(). Add check for dev being
>>>>> NULL in dev_hard_header() to avoid GPF.
>>>>>
>>>>> general protection fault, probably for non-canonical address
>>>>>       0xdffffc0000000046: 0000 [#1] PREEMPT SMP KASAN NOPTI
>>>>> KASAN: null-ptr-deref in range [0x0000000000000230-0x0000000000000237]
>>>>> CPU: 1 PID: 45 Comm: kworker/1:1 Not tainted 6.1.0-rc7+ #2
>>>>> Hardware name: Red Hat KVM, BIOS 1.15.0-2.module+el8.6.0+20659+3dcf7c70
>>>>> Workqueue: mld mld_ifc_work
>>>>> RIP: 0010:macvlan_hard_header (./include/linux/netdevice.h:3057
>>>>>       (discriminator 4) drivers/net/macvlan.c:594 (discriminator 4))
>>>>> RSP: 0018:ffff888103d377d0 EFLAGS: 00010212
>>>>> RAX: dffffc0000000000 RBX: ffff88801cf1a000 RCX: 0000000000000000
>>>>> RDX: 0000000000000046 RSI: 0000000000000000 RDI: 0000000000000230
>>>>> RBP: ffff88801e8ef328 R08: 0000000000000000 R09: 0000000000000060
>>>>> R10: 0000000000000000 R11: 0000000000000000 R12: ffff88801f0497c0
>>>>> R13: 0000000000000000 R14: ffff888045187c98 R15: 0000000000000060
>>>>> FS:  0000000000000000(0000) GS:ffff888106c80000(0000)
>>>>>       knlGS:0000000000000000
>>>>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> CR2: 00007fbf3f1c1840 CR3: 0000000014e36000 CR4: 00000000000006e0
>>>>> Call Trace:
>>>>>    <TASK>
>>>>> neigh_connected_output (./include/linux/netdevice.h:3060
>>>>>       net/core/neighbour.c:1595)
>>>>> ip6_finish_output2 (./include/net/neighbour.h:546
>>>>>       net/ipv6/ip6_output.c:134)
>>>>> ip6_finish_output (net/ipv6/ip6_output.c:195 net/ipv6/ip6_output.c:206)
>>>>> ip6_output (./include/linux/netfilter.h:291 net/ipv6/ip6_output.c:227)
>>>>> NF_HOOK.constprop.0 (./include/net/dst.h:445
>>>>>       ./include/linux/netfilter.h:302)
>>>>> mld_sendpack (net/ipv6/mcast.c:1824)
>>>>> mld_send_cr (net/ipv6/mcast.c:2122)
>>>>> mld_ifc_work (net/ipv6/mcast.c:2655)
>>>>> process_one_work (kernel/workqueue.c:2294)
>>>>> worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2437)
>>>>> kthread (kernel/kthread.c:376)
>>>>> ret_from_fork (arch/x86/entry/entry_64.S:312)
>>>>>    </TASK>
>>>>> Modules linked in:
>>>>> Dumping ftrace buffer:
>>>>>      (ftrace buffer empty)
>>>>> ---[ end trace 0000000000000000 ]---
>>>>>
>>>>> Fixes: 0c4e85813d0a ("[NET]: Wrap netdevice hardware header creation.")
>>>>> Reported-by: syzkaller <syzkaller@googlegroups.com>
>>>>> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
>>>>> ---
>>>>>    include/linux/netdevice.h | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>> index eddf8ee270e7..9b25a6301fa5 100644
>>>>> --- a/include/linux/netdevice.h
>>>>> +++ b/include/linux/netdevice.h
>>>>> @@ -3054,7 +3054,7 @@ static inline int dev_hard_header(struct sk_buff *skb, struct net_device *dev,
>>>>>                                     const void *daddr, const void *saddr,
>>>>>                                     unsigned int len)
>>>>>    {
>>>>> -       if (!dev->header_ops || !dev->header_ops->create)
>>>>> +       if (!dev || !dev->header_ops || !dev->header_ops->create)
>>> Do  you have a repro ?
>> See syzkaller repros attached.
>>
>>> This patch will not prevent a crash later I think.
>> The repro ran overnight without failure with the patch applied.
> Yes, but the patch is hiding a potential bug that might show up with
> other 'repros'
The repro fails when these devices are configured (seem like small mtu):

20: vxcan0@vxcan1: <NOARP,UP,LOWER_UP> mtu 72 qdisc noqueue state UP group default qlen 1000
     link/can
     inet 172.20.20.38/24 scope global vxcan0
        valid_lft forever preferred_lft forever
21: vxcan1@vxcan0: <NOARP,UP,LOWER_UP> mtu 72 qdisc noqueue state UP group default qlen 1000
     link/can
     inet 172.20.20.39/24 scope global vxcan1
        valid_lft forever preferred_lft forever


# diff ../config.fail .config
3325c3325
< CONFIG_CAN_VXCAN=y
---
> # CONFIG_CAN_VXCAN is not set

Thanks,
George

>>> Please fix the root cause, thanks !
>> Will try.
> Thanks, having a repro definitely should help to find the real bug.
>
> I took a look at macvlan , and could not see how vlan->lowerdev  could
> be NULL in the first place.
>
>> Thanks,
>> George
>>>>>                   return 0;
>>>> net_device being NULL during eth header construction? seems like a
>>>> more serious issue?
>>>> If it indeed is a genuine scenario I think a better description is needed...
>>>>
>>>>>           return dev->header_ops->create(skb, dev, type, daddr, saddr, len);
>>>>> --
>>>>> 2.31.1
>>>>>