All of lore.kernel.org
 help / color / mirror / Atom feed
* [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] 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

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

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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.