netdev - Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20231205093910.avpu5udgsahsahoi@skbuf>
Date: Tue, 5 Dec 2023 11:39:10 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Daniel Danzberger <dd@...edd.com>
Cc: woojung.huh@...rochip.com, UNGLinuxDriver@...rochip.com,
	netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on
 platform init

On Tue, Dec 05, 2023 at 10:08:39AM +0100, Daniel Danzberger wrote:
> That module is just a way to instantiate the switch on a piece of fixed custom hardware glued
> together on my desk. It's never meant to be upstream. I just posted it as an example on how the
> switch can be instantiated via 'i2c_new_client_device' instead of DTS.

You're free to use the code in whichever way you want. I was just pointing
out that non-mainline code gets no attention in terms of converting after
breaking changes in the API.

For the networking subsystem there are 2 git trees: net-next for new
features and net for bug fixes which get backported to stable kernels.
In Documentation/process/stable-kernel-rules.rst there are some guidelines
about what constitutes a bug, but my understanding of it is that if we
can't point to some mainline code that is broken, then it's not a bug
and the change goes to net-next (aka work pending for v6.8).

So ok, it's not mainline, perfectly fair, but I want to be clear about
the implication in terms of reduced level of effort in helping to
maintain such support.

Usually, patches are designated by the author in the email title as
"[PATCH net-next]" or "[PATCH net]", and to help the backport, one
should also add a Fixes: tag in case of a bug (search the git log for
examples). You didn't make your intent clear, so I suppose you don't
have a clear expectation of how the patch should be handled. The default
in this case is "net-next".

> The pointer is only copied around, but ksz_platform_data is never actually accessed in any
> meaningful way. The chip_id assigned from DTS or platform_data doesn't even seem to be respected
> anywhere in the decision making.

The one assigned from DTS gets respected in ksz_check_device_id(). If
different from the detected one, the driver fails to probe. Incidentally,
that's also the snag that you hit in your platform_data case.
of_device_get_match_data() doesn't work, so the driver isn't able to
compare the detected switch ID with a pre-established switch ID that it
expects.

Your patch implies it's ok to go along with whatever is detected, which
makes the code paths slightly different. If there is an early error in
the SPI/I2C bus communication, it will be detected for sure in the OF
case, but it might or might not get detected in the platform_data case.
It all depends on what we read and the branches that ksz_switch_detect()
takes.

> Right at 'ksz_switch_register', 'ksz_switch_detect' is called and overwrites 'dev->chip_id' with the
> id read from the hardware:
> 
> static int ksz_switch_detect(struct ksz_device *dev)
> {
> 	u8 id1, id2, id4;
> 	u16 id16;
> 	u32 id32;
> 	int ret;
> 
> 	/* read chip id */
> 	ret = ksz_read16(dev, REG_CHIP_ID0, &id16);
> 	if (ret)
> 		return ret;

Ok. It would be nice to remove the bogus and confusing handling of
struct ksz_platform_data. If someone who has the hardware to test and
make sure nothing breaks (aka you) could do it, it would be even better.