[PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event_rcv()

[PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event_rcv()

[Lin Ma]

This code assumes ifindex provided by user always result in device of
gdb netdev type. Without proper type checking, the casting to nic type
could cause type confusion.

Example crash trace shown as below:

[   49.516445] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   49.520016] #PF: supervisor read access in kernel mode
[   49.520397] #PF: error_code(0x0000) - not-present page
[   49.520780] PGD 0 P4D 0
[   49.520997] Oops: 0000 [#1] PREEMPT SMP NOPTI
[   49.521324] CPU: 0 PID: 172 Comm: trigger Tainted: G         C         6.1.90 #2
[   49.521877] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
[   49.522709] RIP: 0010:gdm_lte_event_rcv+0x17/0x40 [gdmulte]
[   49.523121] Code: 48 89 e5 5d 31 ff c3 cc cc cc cc 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 8b 87 c8 09 00 00 48 89 d6 45 31 c0 89 ca 31 c9 <48> 8b 38 48 8b 40 28 48 89 e5 e8 3a 10 00 c2 5d 31 c0 31 d2 31 c9
[   49.524533] RSP: 0018:ffffc900006879d0 EFLAGS: 00010246
[   49.524929] RAX: 0000000000000000 RBX: ffff888004af8000 RCX: 0000000000000000
[   49.525461] RDX: 0000000000000010 RSI: ffff888004aa1e14 RDI: ffff888004af8000
[   49.525958] RBP: ffffc90000687a08 R08: 0000000000000000 R09: 0000000000000000
[   49.526487] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000010
[   49.527029] R13: ffff888004aa1e00 R14: 0000000000000001 R15: ffff888004aa1e14
[   49.527567] FS:  00007e6a8ab2c740(0000) GS:ffff88807dc00000(0000) knlGS:0000000000000000
[   49.528122] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   49.528544] CR2: 0000000000000000 CR3: 0000000005cc2000 CR4: 00000000000006f0
[   49.529033] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   49.529615] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   49.530120] Call Trace:
[   49.530310]  <TASK>
[   49.530492]  ? __die_body.cold+0x1a/0x1f
[   49.530790]  ? __die+0x2a/0x3b
[   49.531024]  ? page_fault_oops+0x170/0x2f0
[   49.531387]  ? gdm_lte_event_rcv+0x17/0x40 [gdmulte]
[   49.531735]  ? kernelmode_fixup_or_oops+0xb2/0x140
[   49.532105]  ? __bad_area_nosemaphore+0x197/0x1f0
[   49.532492]  ? find_vma+0x30/0x50
[   49.532739]  ? bad_area_nosemaphore+0x16/0x30
[   49.533072]  ? do_user_addr_fault+0x2a8/0x5a0
[   49.533403]  ? exc_page_fault+0x80/0x1b0
[   49.533731]  ? asm_exc_page_fault+0x27/0x30
[   49.534034]  ? gdm_lte_event_rcv+0x17/0x40 [gdmulte]
[   49.534412]  ? netlink_rcv+0x9a/0xd0 [gdmulte]
[   49.534748]  netlink_unicast+0x24d/0x390
[   49.535059]  netlink_sendmsg+0x25e/0x4d0
[   49.535350]  __sock_sendmsg+0x6d/0x70
[   49.535664]  __sys_sendto+0x151/0x1b0
[   49.535958]  __x64_sys_sendto+0x24/0x40
[   49.536236]  x64_sys_call+0x18d4/0x21b0
[   49.536533]  do_syscall_64+0x56/0x90
[   49.536831]  ? kmem_cache_alloc+0x180/0x340
[   49.537132]  ? security_file_alloc+0x2e/0xf0
[   49.537500]  ? apparmor_file_alloc_security+0x40/0x1e0
[   49.537897]  ? __wake_up_common_lock+0x8b/0xd0
[   49.538237]  ? __check_object_size+0x71/0x260
[   49.538623]  ? _copy_to_user+0x25/0x60
[   49.538920]  ? move_addr_to_user+0x53/0xe0
[   49.539228]  ? __sys_getsockname+0xa8/0x110
[   49.539567]  ? exit_to_user_mode_prepare+0x49/0x230
[   49.539958]  ? syscall_exit_to_user_mode+0x22/0x60
[   49.540307]  ? do_syscall_64+0x62/0x90
[   49.540618]  ? lock_mm_and_find_vma+0x43/0x220
[   49.540945]  ? exit_to_user_mode_prepare+0x49/0x230
[   49.541304]  ? irqentry_exit_to_user_mode+0x10/0x30
[   49.541685]  ? irqentry_exit+0x43/0x50
[   49.541995]  ? exc_page_fault+0x91/0x1b0
[   49.542316]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[   49.542666] RIP: 0033:0x7e6a8abb6407

This bug was "fixed" in upstream kernel by the commit 1c2d364e7f7f
("staging: gdm724x: Remove unused driver"). However, other stable
versions still contain it. Fix the confusion bug by adding checks.

Cc: stable@kernel.org
Fixes: 61e121047645 ("staging: gdm7240: adding LTE USB driver")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 drivers/staging/gdm724x/gdm_lte.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/gdm724x/gdm_lte.c b/drivers/staging/gdm724x/gdm_lte.c
index 1f0283fc1d2c..73c7bea16b80 100644
--- a/drivers/staging/gdm724x/gdm_lte.c
+++ b/drivers/staging/gdm724x/gdm_lte.c
@@ -522,6 +522,9 @@ static void gdm_lte_event_rcv(struct net_device *dev, u16 type,
 {
 	struct nic *nic = netdev_priv(dev);
 
+	if (dev->netdev_ops->ndo_open != gdm_lte_open)
+		return;
+
 	nic->phy_dev->send_hci_func(nic->phy_dev->priv_dev, msg, len, NULL,
 				    NULL);
 }
-- 
2.17.1

Re: [PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event_rcv()

[Greg KH]

On Thu, Jul 03, 2025 at 01:27:23PM +0800, Lin Ma wrote:
> This code assumes ifindex provided by user always result in device of
> gdb netdev type. Without proper type checking, the casting to nic type
> could cause type confusion.
> 
> Example crash trace shown as below:
> 
> [   49.516445] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [   49.520016] #PF: supervisor read access in kernel mode
> [   49.520397] #PF: error_code(0x0000) - not-present page
> [   49.520780] PGD 0 P4D 0
> [   49.520997] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [   49.521324] CPU: 0 PID: 172 Comm: trigger Tainted: G         C         6.1.90 #2
> [   49.521877] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> [   49.522709] RIP: 0010:gdm_lte_event_rcv+0x17/0x40 [gdmulte]
> [   49.523121] Code: 48 89 e5 5d 31 ff c3 cc cc cc cc 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 8b 87 c8 09 00 00 48 89 d6 45 31 c0 89 ca 31 c9 <48> 8b 38 48 8b 40 28 48 89 e5 e8 3a 10 00 c2 5d 31 c0 31 d2 31 c9
> [   49.524533] RSP: 0018:ffffc900006879d0 EFLAGS: 00010246
> [   49.524929] RAX: 0000000000000000 RBX: ffff888004af8000 RCX: 0000000000000000
> [   49.525461] RDX: 0000000000000010 RSI: ffff888004aa1e14 RDI: ffff888004af8000
> [   49.525958] RBP: ffffc90000687a08 R08: 0000000000000000 R09: 0000000000000000
> [   49.526487] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000010
> [   49.527029] R13: ffff888004aa1e00 R14: 0000000000000001 R15: ffff888004aa1e14
> [   49.527567] FS:  00007e6a8ab2c740(0000) GS:ffff88807dc00000(0000) knlGS:0000000000000000
> [   49.528122] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   49.528544] CR2: 0000000000000000 CR3: 0000000005cc2000 CR4: 00000000000006f0
> [   49.529033] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   49.529615] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [   49.530120] Call Trace:
> [   49.530310]  <TASK>
> [   49.530492]  ? __die_body.cold+0x1a/0x1f
> [   49.530790]  ? __die+0x2a/0x3b
> [   49.531024]  ? page_fault_oops+0x170/0x2f0
> [   49.531387]  ? gdm_lte_event_rcv+0x17/0x40 [gdmulte]
> [   49.531735]  ? kernelmode_fixup_or_oops+0xb2/0x140
> [   49.532105]  ? __bad_area_nosemaphore+0x197/0x1f0
> [   49.532492]  ? find_vma+0x30/0x50
> [   49.532739]  ? bad_area_nosemaphore+0x16/0x30
> [   49.533072]  ? do_user_addr_fault+0x2a8/0x5a0
> [   49.533403]  ? exc_page_fault+0x80/0x1b0
> [   49.533731]  ? asm_exc_page_fault+0x27/0x30
> [   49.534034]  ? gdm_lte_event_rcv+0x17/0x40 [gdmulte]
> [   49.534412]  ? netlink_rcv+0x9a/0xd0 [gdmulte]
> [   49.534748]  netlink_unicast+0x24d/0x390
> [   49.535059]  netlink_sendmsg+0x25e/0x4d0
> [   49.535350]  __sock_sendmsg+0x6d/0x70
> [   49.535664]  __sys_sendto+0x151/0x1b0
> [   49.535958]  __x64_sys_sendto+0x24/0x40
> [   49.536236]  x64_sys_call+0x18d4/0x21b0
> [   49.536533]  do_syscall_64+0x56/0x90
> [   49.536831]  ? kmem_cache_alloc+0x180/0x340
> [   49.537132]  ? security_file_alloc+0x2e/0xf0
> [   49.537500]  ? apparmor_file_alloc_security+0x40/0x1e0
> [   49.537897]  ? __wake_up_common_lock+0x8b/0xd0
> [   49.538237]  ? __check_object_size+0x71/0x260
> [   49.538623]  ? _copy_to_user+0x25/0x60
> [   49.538920]  ? move_addr_to_user+0x53/0xe0
> [   49.539228]  ? __sys_getsockname+0xa8/0x110
> [   49.539567]  ? exit_to_user_mode_prepare+0x49/0x230
> [   49.539958]  ? syscall_exit_to_user_mode+0x22/0x60
> [   49.540307]  ? do_syscall_64+0x62/0x90
> [   49.540618]  ? lock_mm_and_find_vma+0x43/0x220
> [   49.540945]  ? exit_to_user_mode_prepare+0x49/0x230
> [   49.541304]  ? irqentry_exit_to_user_mode+0x10/0x30
> [   49.541685]  ? irqentry_exit+0x43/0x50
> [   49.541995]  ? exc_page_fault+0x91/0x1b0
> [   49.542316]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> [   49.542666] RIP: 0033:0x7e6a8abb6407
> 
> This bug was "fixed" in upstream kernel by the commit 1c2d364e7f7f
> ("staging: gdm724x: Remove unused driver"). However, other stable
> versions still contain it. Fix the confusion bug by adding checks.
> 
> Cc: stable@kernel.org
> Fixes: 61e121047645 ("staging: gdm7240: adding LTE USB driver")
> Signed-off-by: Lin Ma <linma@zju.edu.cn>
> ---
>  drivers/staging/gdm724x/gdm_lte.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/gdm724x/gdm_lte.c b/drivers/staging/gdm724x/gdm_lte.c
> index 1f0283fc1d2c..73c7bea16b80 100644
> --- a/drivers/staging/gdm724x/gdm_lte.c
> +++ b/drivers/staging/gdm724x/gdm_lte.c
> @@ -522,6 +522,9 @@ static void gdm_lte_event_rcv(struct net_device *dev, u16 type,
>  {
>  	struct nic *nic = netdev_priv(dev);
>  
> +	if (dev->netdev_ops->ndo_open != gdm_lte_open)
> +		return;

Why should a driver be poking around in netdev_ops?  That feels wrong,
what would ever change this?  Why not fix that instead?

thanks,

greg k-h

Re: [PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event_rcv()

[Lin Ma]

Hello Greg,

> > @@ -522,6 +522,9 @@ static void gdm_lte_event_rcv(struct net_device *dev, u16 type,
> >  {
> >  	struct nic *nic = netdev_priv(dev);
> >  
> > +	if (dev->netdev_ops->ndo_open != gdm_lte_open)
> > +		return;
> 
> Why should a driver be poking around in netdev_ops?  That feels wrong,
> what would ever change this?  Why not fix that instead?
> 
> thanks,
> 
> greg k-h

Yes, it seems that comparing ops is quite ambiguous and weird. However, investigation on
code shows various checks against net_device.

- by `dev->priv_flags`. See is_vlan_dev()
```
static inline bool is_vlan_dev(const struct net_device *dev)
{
        return dev->priv_flags & IFF_802_1Q_VLAN;
}
```

- by `dev->rtnl_link_ops->kind`. See lookup_interface()
```
static struct wg_device *lookup_interface(struct nlattr **attrs,
                      struct sk_buff *skb)
{
    dev = dev_get_by_index(sock_net(skb->sk),
                       nla_get_u32(attrs[WGDEVICE_A_IFINDEX]));
    ...
    if (!dev->rtnl_link_ops || !dev->rtnl_link_ops->kind ||
        strcmp(dev->rtnl_link_ops->kind, KBUILD_MODNAME)) {
    ...
}
```

- by `dev->type`. See ax25_device_event()
```
static int ax25_device_event(struct notifier_block *this, unsigned long event,
                 void *ptr)
{
    struct net_device *dev = netdev_notifier_info_to_dev(ptr);

    ......

    /* Reject non AX.25 devices */
    if (dev->type != ARPHRD_AX25)
        return NOTIFY_DONE;
```

As for poking around in netdev_ops, we didn't know if it is the best solution,
just because some code did similar checks, e.g.

```
static struct team *team_nl_team_get(struct genl_info *info)
{
    dev = dev_get_by_index(net, ifindex);
    if (!dev || dev->netdev_ops != &team_netdev_ops) {
        ... 
    }
}
```

and

```
static int mlx5e_set_vf_tunnel(...)
{
    route_dev = dev_get_by_index(dev_net(out_dev), route_dev_ifindex);

    if (!route_dev || route_dev->netdev_ops != &mlx5e_netdev_ops || ... ) {
        ...
    }
}
```

From my point of view, using `dev->type/flags/priv_flags` could be a better
choice if it's okay to introduce more definitions.

Using netdev_ops is more straightforward. Or maybe I should just compare the
`->netdev_ops` instead of the `->netdev_ops->ndo_open`?



Thansk
Lin

Re: [PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event_rcv()

[Greg KH]

On Thu, Jul 03, 2025 at 02:29:40PM +0800, Lin Ma wrote:
> Hello Greg,
> 
> > > @@ -522,6 +522,9 @@ static void gdm_lte_event_rcv(struct net_device *dev, u16 type,
> > >  {
> > >  	struct nic *nic = netdev_priv(dev);
> > >  
> > > +	if (dev->netdev_ops->ndo_open != gdm_lte_open)
> > > +		return;
> > 
> > Why should a driver be poking around in netdev_ops?  That feels wrong,
> > what would ever change this?  Why not fix that instead?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Yes, it seems that comparing ops is quite ambiguous and weird. However, investigation on
> code shows various checks against net_device.
> 
> - by `dev->priv_flags`. See is_vlan_dev()
> ```
> static inline bool is_vlan_dev(const struct net_device *dev)
> {
>         return dev->priv_flags & IFF_802_1Q_VLAN;
> }
> ```
> 
> - by `dev->rtnl_link_ops->kind`. See lookup_interface()
> ```
> static struct wg_device *lookup_interface(struct nlattr **attrs,
>                       struct sk_buff *skb)
> {
>     dev = dev_get_by_index(sock_net(skb->sk),
>                        nla_get_u32(attrs[WGDEVICE_A_IFINDEX]));
>     ...
>     if (!dev->rtnl_link_ops || !dev->rtnl_link_ops->kind ||
>         strcmp(dev->rtnl_link_ops->kind, KBUILD_MODNAME)) {
>     ...
> }
> ```
> 
> - by `dev->type`. See ax25_device_event()
> ```
> static int ax25_device_event(struct notifier_block *this, unsigned long event,
>                  void *ptr)
> {
>     struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> 
>     ......
> 
>     /* Reject non AX.25 devices */
>     if (dev->type != ARPHRD_AX25)
>         return NOTIFY_DONE;
> ```


Those are core functions that all drivers are using, and the "type" of
device is also ok to look at.  You are trying to compare a specific
callback in this change, which feels wrong to me.

Wait, what tree are you making this change against?  I don't even see
the file you are trying to patch in the latest tree, are you sure it's
not just deleted already?

> As for poking around in netdev_ops, we didn't know if it is the best solution,
> just because some code did similar checks, e.g.
> 
> ```
> static struct team *team_nl_team_get(struct genl_info *info)
> {
>     dev = dev_get_by_index(net, ifindex);
>     if (!dev || dev->netdev_ops != &team_netdev_ops) {
>         ... 
>     }
> }
> ```
> 
> and
> 
> ```
> static int mlx5e_set_vf_tunnel(...)
> {
>     route_dev = dev_get_by_index(dev_net(out_dev), route_dev_ifindex);
> 
>     if (!route_dev || route_dev->netdev_ops != &mlx5e_netdev_ops || ... ) {
>         ...
>     }
> }
> ```
> 
> From my point of view, using `dev->type/flags/priv_flags` could be a better
> choice if it's okay to introduce more definitions.
> 
> Using netdev_ops is more straightforward. Or maybe I should just compare the
> `->netdev_ops` instead of the `->netdev_ops->ndo_open`?

Again, make sure this file is still present in the tree before going
further :)

thanks,

greg k-h

Re: [PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event_rcv()

[Lin Ma]

Hello Greg,

> > ```
> > 
> > - by `dev->type`. See ax25_device_event()
> > ```
> > static int ax25_device_event(struct notifier_block *this, unsigned long event,
> >                  void *ptr)
> > {
> >     struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> > 
> >     ......
> > 
> >     /* Reject non AX.25 devices */
> >     if (dev->type != ARPHRD_AX25)
> >         return NOTIFY_DONE;
> > ```
> 
> 
> Those are core functions that all drivers are using, and the "type" of
> device is also ok to look at.  You are trying to compare a specific
> callback in this change, which feels wrong to me.

Got it. I will try to prepare another version.

> 
> Wait, what tree are you making this change against?  I don't even see
> the file you are trying to patch in the latest tree, are you sure it's
> not just deleted already?
> 
> ...
> 
> Again, make sure this file is still present in the tree before going
> further :)
> 
> thanks,
> 
> greg k-h

Yes, you are right, just like how I pointed out in the patch:

"""
This bug was "fixed" in upstream kernel by the commit 1c2d364e7f7f
("staging: gdm724x: Remove unused driver"). However, other stable
versions still contain it. Fix the confusion bug by adding checks.
"""

That is, together with another identified bug, has already been deleted the
latest upstream kernel. (see https://lore.kernel.org/lkml/20250703052837.15458-1-linma@zju.edu.cn/T/#u)

I sent this patch just because the stable version, like 5.15.186, still
contains it. T.T

Therefore, shall I proceed with the patches? Or maybe the stable tree
could also delete that vulnerable code?


Thanks for the reply!


Sincerely
Lin

Re: [PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event_rcv()

[Greg KH]

On Thu, Jul 03, 2025 at 03:03:46PM +0800, Lin Ma wrote:
> Hello Greg,
> 
> > > ```
> > > 
> > > - by `dev->type`. See ax25_device_event()
> > > ```
> > > static int ax25_device_event(struct notifier_block *this, unsigned long event,
> > >                  void *ptr)
> > > {
> > >     struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> > > 
> > >     ......
> > > 
> > >     /* Reject non AX.25 devices */
> > >     if (dev->type != ARPHRD_AX25)
> > >         return NOTIFY_DONE;
> > > ```
> > 
> > 
> > Those are core functions that all drivers are using, and the "type" of
> > device is also ok to look at.  You are trying to compare a specific
> > callback in this change, which feels wrong to me.
> 
> Got it. I will try to prepare another version.
> 
> > 
> > Wait, what tree are you making this change against?  I don't even see
> > the file you are trying to patch in the latest tree, are you sure it's
> > not just deleted already?
> > 
> > ...
> > 
> > Again, make sure this file is still present in the tree before going
> > further :)
> > 
> > thanks,
> > 
> > greg k-h
> 
> Yes, you are right, just like how I pointed out in the patch:
> 
> """
> This bug was "fixed" in upstream kernel by the commit 1c2d364e7f7f
> ("staging: gdm724x: Remove unused driver"). However, other stable
> versions still contain it. Fix the confusion bug by adding checks.
> """
> 
> That is, together with another identified bug, has already been deleted the
> latest upstream kernel. (see https://lore.kernel.org/lkml/20250703052837.15458-1-linma@zju.edu.cn/T/#u)

That email was not threaded, and also not obvious what was going on, so
I long deleted it from my review queue.

> I sent this patch just because the stable version, like 5.15.186, still
> contains it. T.T

Ah, that was not obvious at all.  Remember, some of us get thousands of
emails a day to review, please make things that you are submitting for
non-mainline kernels very very very obvious as to what is going on.

> Therefore, shall I proceed with the patches? Or maybe the stable tree
> could also delete that vulnerable code?

For a staging driver, it's not an issue, don't worry about it, no distro
should be using it anyway as no one has the hardware :)

thanks,

greg k-h

Re: [PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event_rcv()

[Lin Ma]

> 
> Ah, that was not obvious at all.  Remember, some of us get thousands of
> emails a day to review, please make things that you are submitting for
> non-mainline kernels very very very obvious as to what is going on.
> 

Gotcha, this is also the first time I encountered a bug that has been fixed
because it was "removed" (but not treated as a security issue) in upstream.
I will explain clearly what's happening and underline the non-mainline
information next time.


> > Therefore, shall I proceed with the patches? Or maybe the stable tree
> > could also delete that vulnerable code?
> 
> For a staging driver, it's not an issue, don't worry about it, no distro
> should be using it anyway as no one has the hardware :)
> 

Cool, glad to know that.

> thanks,
> 
> greg k-h

Thanks
Lin