netdev - Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference in ksz_connect_tag

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231205170951.x76gswlwvq4gqx3k@skbuf>
Date: Tue, 5 Dec 2023 19:09:51 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Sean Nyekjaer <sean@...nix.com>
Cc: woojung.huh@...rochip.com, UNGLinuxDriver@...rochip.com, andrew@...n.ch,
	ceggers@...i.de, netdev@...r.kernel.org
Subject: Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference in
 ksz_connect_tag_protocol()

On Tue, Dec 05, 2023 at 01:46:36PM +0100, Sean Nyekjaer wrote:
> We should check whether the ksz_tagger_data is allocated.
> For example when using DSA_TAG_PROTO_KSZ8795 protocol, ksz_connect() is not
> allocating ksz_tagger_data.
> 
> This avoids the following null pointer dereference:
> Unable to handle kernel NULL pointer dereference at virtual address 00000000 when write
> [00000000] *pgd=00000000
> Internal error: Oops: 817 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 1 PID: 26 Comm: kworker/u5:1 Not tainted 6.6.0
> Hardware name: STM32 (Device Tree Support)
> Workqueue: events_unbound deferred_probe_work_func
> PC is at ksz_connect_tag_protocol+0x40/0x48
> LR is at ksz_connect_tag_protocol+0x3c/0x48
> [ ... ]
>  ksz_connect_tag_protocol from dsa_register_switch+0x9ac/0xee0
>  dsa_register_switch from ksz_switch_register+0x65c/0x828
>  ksz_switch_register from ksz_spi_probe+0x11c/0x168
>  ksz_spi_probe from spi_probe+0x84/0xa8
>  spi_probe from really_probe+0xc8/0x2d8
> 
> Fixes: ab32f56a4100 ("net: dsa: microchip: ptp: add packet transmission timestamping")
> Signed-off-by: Sean Nyekjaer <sean@...nix.com>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 42db7679c360..1b9815418294 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2623,9 +2623,10 @@ static int ksz_connect_tag_protocol(struct dsa_switch *ds,
>  				    enum dsa_tag_protocol proto)
>  {
>  	struct ksz_tagger_data *tagger_data;
> -
> -	tagger_data = ksz_tagger_data(ds);
> -	tagger_data->xmit_work_fn = ksz_port_deferred_xmit;
> +	if (ksz_tagger_data(ds)) {
> +		tagger_data = ksz_tagger_data(ds);
> +		tagger_data->xmit_work_fn = ksz_port_deferred_xmit;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.42.0
> 
> 

Please spell out the list of protocols for which the driver should
provide its deferred xmit handler. This is what the "enum dsa_tag_protocol
proto" argument is there for. AKA those struct dsa_device_ops that do
provide a "connect" method: DSA_TAG_PROTO_KSZ9477, DSA_TAG_PROTO_KSZ9893,
DSA_TAG_PROTO_LAN937X. Also look at how other drivers do it. My bad for
not spotting this during review of the blamed change.

Simply checking against NULL might be masking other problems and making
them harder to spot.

General process-related information:
- Please designate the next email submission as "PATCH v2 net" to clarify
  that you are targeting the net.git tree for stable fixes, and not the
  net-next.git tree for new features
- Please use ./scripts/get_maintainer.pl more diligently and copy all
  the listed maintainers to future submissions.

---
pw-bot: changes-requested