Re: [PATCH v2] misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge

[Tong Zhang]

On Tue, May 11, 2021 at 11:24 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > Co-Developed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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