netdev - net/hsr: Question about hsr_port_get_hsr() and possbile null-pointer-dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CANdh5G7KBdzVcyrf5dPG2fbXQ5KCzr0LXu_p38H2-Cd4_FNsxw@mail.gmail.com>
Date: Wed, 2 Oct 2024 10:51:52 -0500
From: Zichen Xie <zichenxie0106@...il.com>
To: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, horms@...nel.org, lukma@...x.de, 
	aleksander.lobakin@...el.com, n.zhandarovich@...tech.ru, ricardo@...liere.net, 
	m-karicheri2@...com
Cc: netdev@...r.kernel.org, Zijie Zhao <zzjas98@...il.com>, 
	Chenyuan Yang <chenyuan0y@...il.com>
Subject: net/hsr: Question about hsr_port_get_hsr() and possbile null-pointer-dereference

Dear Developers for NETWORKING [GENERAL],

We are curious about the function hsr_port_get_hsr().
The function may return NULL when it cannot find a corresponding port.
But there is no NULL check in hsr_check_carrier_and_operstate() here:
https://elixir.bootlin.com/linux/v6.12-rc1/source/net/hsr/hsr_device.c#L93
The relevant code is:
```
master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
/* netif_stacked_transfer_operstate() cannot be used here since
* it doesn't set IF_OPER_LOWERLAYERDOWN (?)
*/
has_carrier = hsr_check_carrier(master);
hsr_set_operstate(master, has_carrier);
hsr_check_announce(master->dev);
```
There may be possible NULL Pointer Dereference.
However, in hsr_dev_xmit() the NULL checker exists.
```
master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
if (master) {
skb->dev = master->dev;
skb_reset_mac_header(skb);
skb_reset_mac_len(skb);
spin_lock_bh(&hsr->seqnr_lock);
hsr_forward_skb(skb, master);
spin_unlock_bh(&hsr->seqnr_lock);
} else {
dev_core_stats_tx_dropped_inc(dev);
dev_kfree_skb_any(skb);
}
```
So we are curious if this NULL check is necessary. The function
hsr_port_get_hsr() is called several times, but NULL checks seem to
exist occasionally.

Please kindly correct us if we missed any key information. Looking
forward to your response!

Best,
Zichen