| Message ID | 20231205124636.1345761-1-sean@geanix.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: fix NULL pointer dereference in ksz_connect_tag_protocol() | expand |
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@geanix.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
Hi Sean, > 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; > + } Do we need to return error in case of xmit_work_fn is not initialized. > > return 0; > } > -- > 2.42.0
Hi Sean -----Original Message----- From: Sean Nyekjaer <sean@geanix.com> Sent: Tuesday, December 5, 2023 6:17 PM To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver <UNGLinuxDriver@microchip.com>; andrew@lunn.ch; ceggers@arri.de; netdev@vger.kernel.org Cc: Sean Nyekjaer <sean@geanix.com> Subject: [PATCH] net: dsa: microchip: fix NULL pointer dereference in ksz_connect_tag_protocol() [Some people who received this message don't often get email from sean@geanix.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe 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@geanix.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 Instead of calling " ksz_tagger_data(ds)" twice, NULL check tagger_data before assigning " xmit_work_fn" would help right? Is there any reason for doing this way?
> On 6 Dec 2023, at 14.35, <Madhuri.Sripada@microchip.com> <Madhuri.Sripada@microchip.com> wrote: > > Hi Sean > > -----Original Message----- > From: Sean Nyekjaer <sean@geanix.com> > Sent: Tuesday, December 5, 2023 6:17 PM > To: Woojung Huh - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver <UNGLinuxDriver@microchip.com>; andrew@lunn.ch; ceggers@arri.de; netdev@vger.kernel.org > Cc: Sean Nyekjaer <sean@geanix.com> > Subject: [PATCH] net: dsa: microchip: fix NULL pointer dereference in ksz_connect_tag_protocol() > > [Some people who received this message don't often get email from sean@geanix.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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@geanix.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 > > > Instead of calling " ksz_tagger_data(ds)" twice, NULL check tagger_data before assigning " xmit_work_fn" would help right? > Is there any reason for doing this way? I have submitted a v2 of this patch: https://lore.kernel.org/netdev/20231206071655.1626479-1-sean@geanix.com/T/#u /Sean
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; }
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@geanix.com> --- drivers/net/dsa/microchip/ksz_common.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)