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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Mon, May 10, 2021 at 7:36 AM Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
> > ---
> > 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
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
> <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
On Tue, May 11, 2021 at 12:03 AM Greg Kroah-Hartman
<[email protected]> 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
> > <[email protected]> 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
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
> <[email protected]> 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
> > > <[email protected]> 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
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 <[email protected]>
Co-Developed-by: Greg Kroah-Hartman <[email protected]>
---
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
On Tue, May 11, 2021 at 10:57 AM Greg Kroah-Hartman
<[email protected]> 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
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 <[email protected]>
> Co-Developed-by: Greg Kroah-Hartman <[email protected]>
> ---
> 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
On Tue, May 11, 2021 at 11:24 PM Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
> > Co-Developed-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > 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
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
> <[email protected]> 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 <[email protected]>
> > > Co-Developed-by: Greg Kroah-Hartman <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
Co-developed-by: Greg Kroah-Hartman <[email protected]>
---
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
On Wed, May 12, 2021 at 9:41 AM Greg Kroah-Hartman
<[email protected]> 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
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
On Wed, May 19, 2021 at 1:40 AM Dan Carpenter <[email protected]> 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