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

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

[Zichen Xie]

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

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

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

Hi Zichen,

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

This function is called when NETDEV_UP/DOWN/CHANGE is called for hsr
net device.

IMHO, this cannot be called without having first created hsr network
device (with iproute2 command).

> ```
> 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




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]