* [PATCH] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge @ 2021-04-26 22:07 Tong Zhang 2021-05-10 14:36 ` Greg Kroah-Hartman 0 siblings, 1 reply; 15+ messages in thread From: Tong Zhang @ 2021-04-26 22:07 UTC (permalink / raw) To: Arnd Bergmann, Greg Kroah-Hartman, Tong Zhang, linux-kernel the PCI bridge might be NULL, so we'd better check before use it [ 1.246492] BUG: kernel NULL pointer dereference, address: 00000000000000c0 [ 1.248731] RIP: 0010:pci_read_config_byte+0x5/0x40 [ 1.253998] Call Trace: [ 1.254131] ? alcor_pci_find_cap_offset.isra.0+0x3a/0x100 [alcor_pci] [ 1.254476] alcor_pci_probe+0x169/0x2d5 [alcor_pci] Signed-off-by: Tong Zhang <ztong0001@gmail.com> --- drivers/misc/cardreader/alcor_pci.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/misc/cardreader/alcor_pci.c b/drivers/misc/cardreader/alcor_pci.c index cd402c89189e..1c33453fd5c7 100644 --- a/drivers/misc/cardreader/alcor_pci.c +++ b/drivers/misc/cardreader/alcor_pci.c @@ -102,6 +102,9 @@ static int alcor_pci_find_cap_offset(struct alcor_pci_priv *priv, u8 val8; u32 val32; + if (!pci) + return 0; + where = ALCOR_CAP_START_OFFSET; pci_read_config_byte(pci, where, &val8); if (!val8) -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-04-26 22:07 [PATCH] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge Tong Zhang @ 2021-05-10 14:36 ` Greg Kroah-Hartman 2021-05-10 22:20 ` Tong Zhang 0 siblings, 1 reply; 15+ messages in thread From: Greg Kroah-Hartman @ 2021-05-10 14:36 UTC (permalink / raw) To: Tong Zhang; +Cc: Arnd Bergmann, linux-kernel On Mon, Apr 26, 2021 at 06:07:27PM -0400, Tong Zhang wrote: > the PCI bridge might be NULL, so we'd better check before use it > > [ 1.246492] BUG: kernel NULL pointer dereference, address: 00000000000000c0 > [ 1.248731] RIP: 0010:pci_read_config_byte+0x5/0x40 > [ 1.253998] Call Trace: > [ 1.254131] ? alcor_pci_find_cap_offset.isra.0+0x3a/0x100 [alcor_pci] > [ 1.254476] alcor_pci_probe+0x169/0x2d5 [alcor_pci] > > Signed-off-by: Tong Zhang <ztong0001@gmail.com> > --- > drivers/misc/cardreader/alcor_pci.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/misc/cardreader/alcor_pci.c b/drivers/misc/cardreader/alcor_pci.c > index cd402c89189e..1c33453fd5c7 100644 > --- a/drivers/misc/cardreader/alcor_pci.c > +++ b/drivers/misc/cardreader/alcor_pci.c > @@ -102,6 +102,9 @@ static int alcor_pci_find_cap_offset(struct alcor_pci_priv *priv, > u8 val8; > u32 val32; > > + if (!pci) > + return 0; > + > where = ALCOR_CAP_START_OFFSET; > pci_read_config_byte(pci, where, &val8); > if (!val8) > -- > 2.25.1 > I do not understand, how can pci ever be NULL? There is only 1 way this function can be called, and it's through the alcor_pci_probe() call, which should have always set up the parent and pci pointers that get passed to this function. How can that not happen? If it can happen, then something earlier than this should be fixed instead of papering over the root problem here. How did you duplicate the crash you list above? thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-10 14:36 ` Greg Kroah-Hartman @ 2021-05-10 22:20 ` Tong Zhang 2021-05-11 7:03 ` Greg Kroah-Hartman 0 siblings, 1 reply; 15+ messages in thread From: Tong Zhang @ 2021-05-10 22:20 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Arnd Bergmann, open list On Mon, May 10, 2021 at 7:36 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Apr 26, 2021 at 06:07:27PM -0400, Tong Zhang wrote: > > the PCI bridge might be NULL, so we'd better check before use it > > > > [ 1.246492] BUG: kernel NULL pointer dereference, address: 00000000000000c0 > > [ 1.248731] RIP: 0010:pci_read_config_byte+0x5/0x40 > > [ 1.253998] Call Trace: > > [ 1.254131] ? alcor_pci_find_cap_offset.isra.0+0x3a/0x100 [alcor_pci] > > [ 1.254476] alcor_pci_probe+0x169/0x2d5 [alcor_pci] > > > > Signed-off-by: Tong Zhang <ztong0001@gmail.com> > > --- > > drivers/misc/cardreader/alcor_pci.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/misc/cardreader/alcor_pci.c b/drivers/misc/cardreader/alcor_pci.c > > index cd402c89189e..1c33453fd5c7 100644 > > --- a/drivers/misc/cardreader/alcor_pci.c > > +++ b/drivers/misc/cardreader/alcor_pci.c > > @@ -102,6 +102,9 @@ static int alcor_pci_find_cap_offset(struct alcor_pci_priv *priv, > > u8 val8; > > u32 val32; > > > > + if (!pci) > > + return 0; > > + > > where = ALCOR_CAP_START_OFFSET; > > pci_read_config_byte(pci, where, &val8); > > if (!val8) > > -- > > 2.25.1 > > > > I do not understand, how can pci ever be NULL? There is only 1 way this Hi Greg, I think the problem is with priv->parent_pdev = pdev->bus->self where bus->self can be NULL. when bus->self is NULL, calling alcor_pci_find_cap_offset(priv, priv->parent_pdev) is equivalent to alcor_pci_find_cap_offset(priv, NULL) - Tong > function can be called, and it's through the alcor_pci_probe() call, > which should have always set up the parent and pci pointers that get > passed to this function. > > How can that not happen? If it can happen, then something earlier than > this should be fixed instead of papering over the root problem here. > > How did you duplicate the crash you list above? > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-10 22:20 ` Tong Zhang @ 2021-05-11 7:03 ` Greg Kroah-Hartman 2021-05-11 17:17 ` Tong Zhang 0 siblings, 1 reply; 15+ messages in thread From: Greg Kroah-Hartman @ 2021-05-11 7:03 UTC (permalink / raw) To: Tong Zhang; +Cc: Arnd Bergmann, open list On Mon, May 10, 2021 at 03:20:02PM -0700, Tong Zhang wrote: > On Mon, May 10, 2021 at 7:36 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Apr 26, 2021 at 06:07:27PM -0400, Tong Zhang wrote: > > > the PCI bridge might be NULL, so we'd better check before use it > > > > > > [ 1.246492] BUG: kernel NULL pointer dereference, address: 00000000000000c0 > > > [ 1.248731] RIP: 0010:pci_read_config_byte+0x5/0x40 > > > [ 1.253998] Call Trace: > > > [ 1.254131] ? alcor_pci_find_cap_offset.isra.0+0x3a/0x100 [alcor_pci] > > > [ 1.254476] alcor_pci_probe+0x169/0x2d5 [alcor_pci] > > > > > > Signed-off-by: Tong Zhang <ztong0001@gmail.com> > > > --- > > > drivers/misc/cardreader/alcor_pci.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/misc/cardreader/alcor_pci.c b/drivers/misc/cardreader/alcor_pci.c > > > index cd402c89189e..1c33453fd5c7 100644 > > > --- a/drivers/misc/cardreader/alcor_pci.c > > > +++ b/drivers/misc/cardreader/alcor_pci.c > > > @@ -102,6 +102,9 @@ static int alcor_pci_find_cap_offset(struct alcor_pci_priv *priv, > > > u8 val8; > > > u32 val32; > > > > > > + if (!pci) > > > + return 0; > > > + > > > where = ALCOR_CAP_START_OFFSET; > > > pci_read_config_byte(pci, where, &val8); > > > if (!val8) > > > -- > > > 2.25.1 > > > > > > > I do not understand, how can pci ever be NULL? There is only 1 way this > > Hi Greg, > I think the problem is with > priv->parent_pdev = pdev->bus->self > where bus->self can be NULL. when bus->self is NULL, calling How can bus->self be NULL? Did you see this on a real system? How did you duplicate the error listed here? thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-11 7:03 ` Greg Kroah-Hartman @ 2021-05-11 17:17 ` Tong Zhang 2021-05-11 17:57 ` Greg Kroah-Hartman 0 siblings, 1 reply; 15+ messages in thread From: Tong Zhang @ 2021-05-11 17:17 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Arnd Bergmann, open list On Tue, May 11, 2021 at 12:03 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, May 10, 2021 at 03:20:02PM -0700, Tong Zhang wrote: > > On Mon, May 10, 2021 at 7:36 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Apr 26, 2021 at 06:07:27PM -0400, Tong Zhang wrote: > > > > the PCI bridge might be NULL, so we'd better check before use it > > > > > > I do not understand, how can pci ever be NULL? There is only 1 way this > > > > Hi Greg, > > I think the problem is with > > priv->parent_pdev = pdev->bus->self > > where bus->self can be NULL. when bus->self is NULL, calling > > How can bus->self be NULL? Hi Greg, Please correct me if I am wrong, when bus->self is not NULL, it means there is a bridge, However, a device can be directly attached to the port on the root complex. In this case, the bus->self is NULL. > Did you see this on a real system? How did you duplicate the error > listed here? I did this in QEMU. If QEMU is considered not real, then I haven't seen an alcor controller configured in this way in a real system. That being said, this kind of configuration is still legit IMHO. Best, - Tong > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-11 17:17 ` Tong Zhang @ 2021-05-11 17:57 ` Greg Kroah-Hartman 2021-05-11 21:29 ` [PATCH v2] " Tong Zhang 2021-05-11 21:32 ` [PATCH] " Tong Zhang 0 siblings, 2 replies; 15+ messages in thread From: Greg Kroah-Hartman @ 2021-05-11 17:57 UTC (permalink / raw) To: Tong Zhang; +Cc: Arnd Bergmann, open list On Tue, May 11, 2021 at 10:17:12AM -0700, Tong Zhang wrote: > On Tue, May 11, 2021 at 12:03 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, May 10, 2021 at 03:20:02PM -0700, Tong Zhang wrote: > > > On Mon, May 10, 2021 at 7:36 AM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Mon, Apr 26, 2021 at 06:07:27PM -0400, Tong Zhang wrote: > > > > > the PCI bridge might be NULL, so we'd better check before use it > > > > > > > > I do not understand, how can pci ever be NULL? There is only 1 way this > > > > > > Hi Greg, > > > I think the problem is with > > > priv->parent_pdev = pdev->bus->self > > > where bus->self can be NULL. when bus->self is NULL, calling > > > > How can bus->self be NULL? > > Hi Greg, > Please correct me if I am wrong, > when bus->self is not NULL, it means there is a bridge, > However, a device can be directly attached to the port on the root > complex. In this case, the bus->self is NULL. Does that ever happen with a device on the root like that? > > Did you see this on a real system? How did you duplicate the error > > listed here? > I did this in QEMU. If QEMU is considered not real, then I haven't > seen an alcor controller configured in this way in a real system. > That being said, this kind of configuration is still legit IMHO. Ah, ok, that makes more sense, this is a virtual system. I suggest, again, steping back up and just not calling this function if you are on the root, as it does not make any sense to do so for a device that is not there. thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-11 17:57 ` Greg Kroah-Hartman @ 2021-05-11 21:29 ` Tong Zhang 2021-05-12 6:24 ` Greg Kroah-Hartman 2021-05-11 21:32 ` [PATCH] " Tong Zhang 1 sibling, 1 reply; 15+ messages in thread From: Tong Zhang @ 2021-05-11 21:29 UTC (permalink / raw) To: Arnd Bergmann, Greg Kroah-Hartman, Tong Zhang, linux-kernel Device might be attached to root complex directly. In this case, bus->self(bridge) will be NULL, so we'd better check before use it [ 1.246492] BUG: kernel NULL pointer dereference, address: 00000000000000c0 [ 1.248731] RIP: 0010:pci_read_config_byte+0x5/0x40 [ 1.253998] Call Trace: [ 1.254131] ? alcor_pci_find_cap_offset.isra.0+0x3a/0x100 [alcor_pci] [ 1.254476] alcor_pci_probe+0x169/0x2d5 [alcor_pci] Signed-off-by: Tong Zhang <ztong0001@gmail.com> Co-Developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- v2: check before calling alcor_pci_find_cap_offset() drivers/misc/cardreader/alcor_pci.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/misc/cardreader/alcor_pci.c b/drivers/misc/cardreader/alcor_pci.c index cd402c89189e..175c6b06f7aa 100644 --- a/drivers/misc/cardreader/alcor_pci.c +++ b/drivers/misc/cardreader/alcor_pci.c @@ -139,6 +139,9 @@ static void alcor_pci_init_check_aspm(struct alcor_pci_priv *priv) u32 val32; priv->pdev_cap_off = alcor_pci_find_cap_offset(priv, priv->pdev); + + if (!priv->parent_pdev) + return; priv->parent_cap_off = alcor_pci_find_cap_offset(priv, priv->parent_pdev); -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-11 21:29 ` [PATCH v2] " Tong Zhang @ 2021-05-12 6:24 ` Greg Kroah-Hartman 2021-05-12 16:24 ` Tong Zhang 0 siblings, 1 reply; 15+ messages in thread From: Greg Kroah-Hartman @ 2021-05-12 6:24 UTC (permalink / raw) To: Tong Zhang; +Cc: Arnd Bergmann, linux-kernel On Tue, May 11, 2021 at 05:29:38PM -0400, Tong Zhang wrote: > Device might be attached to root complex directly. In this case, > bus->self(bridge) will be NULL, so we'd better check before use it > > [ 1.246492] BUG: kernel NULL pointer dereference, address: 00000000000000c0 > [ 1.248731] RIP: 0010:pci_read_config_byte+0x5/0x40 > [ 1.253998] Call Trace: > [ 1.254131] ? alcor_pci_find_cap_offset.isra.0+0x3a/0x100 [alcor_pci] > [ 1.254476] alcor_pci_probe+0x169/0x2d5 [alcor_pci] > > Signed-off-by: Tong Zhang <ztong0001@gmail.com> > Co-Developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > v2: check before calling alcor_pci_find_cap_offset() > > drivers/misc/cardreader/alcor_pci.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/misc/cardreader/alcor_pci.c b/drivers/misc/cardreader/alcor_pci.c > index cd402c89189e..175c6b06f7aa 100644 > --- a/drivers/misc/cardreader/alcor_pci.c > +++ b/drivers/misc/cardreader/alcor_pci.c > @@ -139,6 +139,9 @@ static void alcor_pci_init_check_aspm(struct alcor_pci_priv *priv) > u32 val32; > > priv->pdev_cap_off = alcor_pci_find_cap_offset(priv, priv->pdev); > + > + if (!priv->parent_pdev) > + return; That feels wrong, you just prevented all of the remaining logic in this call to not be set up. Did you test this and did the driver and device still work properly if it hits this check? thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-12 6:24 ` Greg Kroah-Hartman @ 2021-05-12 16:24 ` Tong Zhang 2021-05-12 16:41 ` Greg Kroah-Hartman 0 siblings, 1 reply; 15+ messages in thread From: Tong Zhang @ 2021-05-12 16:24 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Arnd Bergmann, open list On Tue, May 11, 2021 at 11:24 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, May 11, 2021 at 05:29:38PM -0400, Tong Zhang wrote: > > Device might be attached to root complex directly. In this case, > > bus->self(bridge) will be NULL, so we'd better check before use it > > > > [ 1.246492] BUG: kernel NULL pointer dereference, address: 00000000000000c0 > > [ 1.248731] RIP: 0010:pci_read_config_byte+0x5/0x40 > > [ 1.253998] Call Trace: > > [ 1.254131] ? alcor_pci_find_cap_offset.isra.0+0x3a/0x100 [alcor_pci] > > [ 1.254476] alcor_pci_probe+0x169/0x2d5 [alcor_pci] > > > > Signed-off-by: Tong Zhang <ztong0001@gmail.com> > > Co-Developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > v2: check before calling alcor_pci_find_cap_offset() > > > > drivers/misc/cardreader/alcor_pci.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/misc/cardreader/alcor_pci.c b/drivers/misc/cardreader/alcor_pci.c > > index cd402c89189e..175c6b06f7aa 100644 > > --- a/drivers/misc/cardreader/alcor_pci.c > > +++ b/drivers/misc/cardreader/alcor_pci.c > > @@ -139,6 +139,9 @@ static void alcor_pci_init_check_aspm(struct alcor_pci_priv *priv) > > u32 val32; > > > > priv->pdev_cap_off = alcor_pci_find_cap_offset(priv, priv->pdev); > > + > > + if (!priv->parent_pdev) > > + return; > > That feels wrong, you just prevented all of the remaining logic in this > call to not be set up. Did you test this and did the driver and device > still work properly if it hits this check? > > thanks, > > greg k-h Sorry, probably I misunderstood your previous email. Please correct me if I am wrong. What I did here is to disable ASPM completely if it is attached to the root complex, which is OK since ASPM is optional and we cannot really do ASPM on the root complex. Also, alcor_pci_init_check_aspm() is responsible for checking the device and its parent(bridge) aspm capability offset. This function will set priv->parent_cap_off and priv->pdev_cap_off. Those two capability offset will be used in alcor_pci_aspm_ctrl() to determine whether the PCI link+device supports aspm or not. In our case the pdev_cap_off remains 0 when alcor_pci_aspm_ctrl() is called and it simply returns. So I think it can still work. - Tong ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-12 16:24 ` Tong Zhang @ 2021-05-12 16:41 ` Greg Kroah-Hartman 2021-05-13 4:07 ` [PATCH v3] " Tong Zhang 2021-05-13 4:09 ` [PATCH v2] " Tong Zhang 0 siblings, 2 replies; 15+ messages in thread From: Greg Kroah-Hartman @ 2021-05-12 16:41 UTC (permalink / raw) To: Tong Zhang; +Cc: Arnd Bergmann, open list On Wed, May 12, 2021 at 09:24:55AM -0700, Tong Zhang wrote: > On Tue, May 11, 2021 at 11:24 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, May 11, 2021 at 05:29:38PM -0400, Tong Zhang wrote: > > > Device might be attached to root complex directly. In this case, > > > bus->self(bridge) will be NULL, so we'd better check before use it > > > > > > [ 1.246492] BUG: kernel NULL pointer dereference, address: 00000000000000c0 > > > [ 1.248731] RIP: 0010:pci_read_config_byte+0x5/0x40 > > > [ 1.253998] Call Trace: > > > [ 1.254131] ? alcor_pci_find_cap_offset.isra.0+0x3a/0x100 [alcor_pci] > > > [ 1.254476] alcor_pci_probe+0x169/0x2d5 [alcor_pci] > > > > > > Signed-off-by: Tong Zhang <ztong0001@gmail.com> > > > Co-Developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > --- > > > v2: check before calling alcor_pci_find_cap_offset() > > > > > > drivers/misc/cardreader/alcor_pci.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/misc/cardreader/alcor_pci.c b/drivers/misc/cardreader/alcor_pci.c > > > index cd402c89189e..175c6b06f7aa 100644 > > > --- a/drivers/misc/cardreader/alcor_pci.c > > > +++ b/drivers/misc/cardreader/alcor_pci.c > > > @@ -139,6 +139,9 @@ static void alcor_pci_init_check_aspm(struct alcor_pci_priv *priv) > > > u32 val32; > > > > > > priv->pdev_cap_off = alcor_pci_find_cap_offset(priv, priv->pdev); > > > + > > > + if (!priv->parent_pdev) > > > + return; > > > > That feels wrong, you just prevented all of the remaining logic in this > > call to not be set up. Did you test this and did the driver and device > > still work properly if it hits this check? > > > > thanks, > > > > greg k-h > > Sorry, probably I misunderstood your previous email. Please correct me > if I am wrong. > What I did here is to disable ASPM completely if it is attached to the > root complex, which is OK since ASPM is optional and we cannot really > do ASPM on the root complex. > Also, alcor_pci_init_check_aspm() is responsible for checking the > device and its parent(bridge) aspm capability offset. > This function will set priv->parent_cap_off and priv->pdev_cap_off. > Those two capability offset will be used in alcor_pci_aspm_ctrl() to > determine whether the PCI link+device supports aspm or not. > In our case the pdev_cap_off remains 0 when alcor_pci_aspm_ctrl() is > called and it simply returns. > So I think it can still work. Ok, that makes more sense. Can you document that better and add a comment here, and properly handle the whitespace and resubmit? thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-12 16:41 ` Greg Kroah-Hartman @ 2021-05-13 4:07 ` Tong Zhang 2021-05-19 8:40 ` Dan Carpenter 2021-05-13 4:09 ` [PATCH v2] " Tong Zhang 1 sibling, 1 reply; 15+ messages in thread From: Tong Zhang @ 2021-05-13 4:07 UTC (permalink / raw) To: Arnd Bergmann, Greg Kroah-Hartman, Tong Zhang, linux-kernel There is an issue with the ASPM(optional) capability checking function. A device might be attached to root complex directly, in this case, bus->self(bridge) will be NULL, thus priv->parent_pdev is NULL. Since alcor_pci_init_check_aspm(priv->parent_pdev) checks the PCI link's ASPM capability and populate parent_cap_off, which will be used later by alcor_pci_aspm_ctrl() to dynamically turn on/off device, what we can do here is to avoid checking the capability if we are on the root complex. This will make pdev_cap_off 0 and alcor_pci_aspm_ctrl() will simply return when bring called, effectively disable ASPM for the device. [ 1.246492] BUG: kernel NULL pointer dereference, address: 00000000000000c0 [ 1.248731] RIP: 0010:pci_read_config_byte+0x5/0x40 [ 1.253998] Call Trace: [ 1.254131] ? alcor_pci_find_cap_offset.isra.0+0x3a/0x100 [alcor_pci] [ 1.254476] alcor_pci_probe+0x169/0x2d5 [alcor_pci] Signed-off-by: Tong Zhang <ztong0001@gmail.com> Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- v2: check before calling alcor_pci_find_cap_offset() v3: Add comment. Enable the dev_dbg() output when priv->parent_pdev is NULL. drivers/misc/cardreader/alcor_pci.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/misc/cardreader/alcor_pci.c b/drivers/misc/cardreader/alcor_pci.c index cd402c89189e..0a62307f7ffb 100644 --- a/drivers/misc/cardreader/alcor_pci.c +++ b/drivers/misc/cardreader/alcor_pci.c @@ -139,7 +139,13 @@ static void alcor_pci_init_check_aspm(struct alcor_pci_priv *priv) u32 val32; priv->pdev_cap_off = alcor_pci_find_cap_offset(priv, priv->pdev); - priv->parent_cap_off = alcor_pci_find_cap_offset(priv, + /* + * A device might be attached to root complex directly and + * priv->parent_pdev will be NULL. In this case we don't check its + * capability and disable ASPM completely. + */ + if (!priv->parent_pdev) + priv->parent_cap_off = alcor_pci_find_cap_offset(priv, priv->parent_pdev); if ((priv->pdev_cap_off == 0) || (priv->parent_cap_off == 0)) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* re: [PATCH v3] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-13 4:07 ` [PATCH v3] " Tong Zhang @ 2021-05-19 8:40 ` Dan Carpenter 2021-05-19 20:20 ` Tong Zhang 0 siblings, 1 reply; 15+ messages in thread From: Dan Carpenter @ 2021-05-19 8:40 UTC (permalink / raw) To: ztong0001; +Cc: Greg Kroah-Hartman, Arnd Bergmann, open list Hello Tong Zhang, This is a semi-automatic email about new static checker warnings. The patch 3ce3e45cc333: "misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge" from May 13, 2021, leads to the following Smatch complaint: drivers/misc/cardreader/alcor_pci.c:149 alcor_pci_init_check_aspm() error: we previously assumed 'priv->parent_pdev' could be null (see line 147) drivers/misc/cardreader/alcor_pci.c 142 /* 143 * A device might be attached to root complex directly and 144 * priv->parent_pdev will be NULL. In this case we don't check its 145 * capability and disable ASPM completely. 146 */ 147 if (!priv->parent_pdev) ^^^^^^^^^^^^^^^^^^ 148 priv->parent_cap_off = alcor_pci_find_cap_offset(priv, 149 priv->parent_pdev); ^^^^^^^^^^^^^^^^^^ It will just crash inside the function call. Is the if statement reversed? 150 151 if ((priv->pdev_cap_off == 0) || (priv->parent_cap_off == 0)) { regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-19 8:40 ` Dan Carpenter @ 2021-05-19 20:20 ` Tong Zhang 0 siblings, 0 replies; 15+ messages in thread From: Tong Zhang @ 2021-05-19 20:20 UTC (permalink / raw) To: Dan Carpenter; +Cc: Greg Kroah-Hartman, Arnd Bergmann, open list On Wed, May 19, 2021 at 1:40 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > Hello Tong Zhang, > > This is a semi-automatic email about new static checker warnings. > > The patch 3ce3e45cc333: "misc: alcor_pci: fix null-ptr-deref when > there is no PCI bridge" from May 13, 2021, leads to the following > Smatch complaint: > > drivers/misc/cardreader/alcor_pci.c:149 alcor_pci_init_check_aspm() > error: we previously assumed 'priv->parent_pdev' could be null (see line 147) > > drivers/misc/cardreader/alcor_pci.c > 142 /* > 143 * A device might be attached to root complex directly and > 144 * priv->parent_pdev will be NULL. In this case we don't check its > 145 * capability and disable ASPM completely. > 146 */ > 147 if (!priv->parent_pdev) > ^^^^^^^^^^^^^^^^^^ > > 148 priv->parent_cap_off = alcor_pci_find_cap_offset(priv, > 149 priv->parent_pdev); > ^^^^^^^^^^^^^^^^^^ > It will just crash inside the function call. Is the if statement > reversed? > > 150 > 151 if ((priv->pdev_cap_off == 0) || (priv->parent_cap_off == 0)) { > > regards, > dan carpenter Thanks Dan. I already corrected this in v4 https://lkml.org/lkml/2021/5/18/1040 Please check if the issue persists. - Tong ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-12 16:41 ` Greg Kroah-Hartman 2021-05-13 4:07 ` [PATCH v3] " Tong Zhang @ 2021-05-13 4:09 ` Tong Zhang 1 sibling, 0 replies; 15+ messages in thread From: Tong Zhang @ 2021-05-13 4:09 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Arnd Bergmann, open list On Wed, May 12, 2021 at 9:41 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > Can you document that better and add a comment here, and properly handle > the whitespace and resubmit? > > thanks, > > greg k-h Thanks Greg, revised as suggested and sent as v3. - Tong ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge 2021-05-11 17:57 ` Greg Kroah-Hartman 2021-05-11 21:29 ` [PATCH v2] " Tong Zhang @ 2021-05-11 21:32 ` Tong Zhang 1 sibling, 0 replies; 15+ messages in thread From: Tong Zhang @ 2021-05-11 21:32 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Arnd Bergmann, open list On Tue, May 11, 2021 at 10:57 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > I suggest, again, steping back up and just not calling this function if > you are on the root, as it does not make any sense to do so for a device > that is not there. > Agreed. I did a revision based on your suggestion and sent it as v2. Thanks, - Tong > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-05-19 20:20 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-26 22:07 [PATCH] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge Tong Zhang 2021-05-10 14:36 ` Greg Kroah-Hartman 2021-05-10 22:20 ` Tong Zhang 2021-05-11 7:03 ` Greg Kroah-Hartman 2021-05-11 17:17 ` Tong Zhang 2021-05-11 17:57 ` Greg Kroah-Hartman 2021-05-11 21:29 ` [PATCH v2] " Tong Zhang 2021-05-12 6:24 ` Greg Kroah-Hartman 2021-05-12 16:24 ` Tong Zhang 2021-05-12 16:41 ` Greg Kroah-Hartman 2021-05-13 4:07 ` [PATCH v3] " Tong Zhang 2021-05-19 8:40 ` Dan Carpenter 2021-05-19 20:20 ` Tong Zhang 2021-05-13 4:09 ` [PATCH v2] " Tong Zhang 2021-05-11 21:32 ` [PATCH] " Tong Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).