PCI/AER: potential dereference of null pointer

he return value of kzalloc() needs to be checked.
To avoid use of null pointer in case of the failure of alloc.

Fixes: db89ccbe52c7 ("PCI/AER: Define aer_stats structure for AER capable devices")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 drivers/pci/pcie/aer.c | 2 ++
 1 file changed, 2 insertions(+)

[+cc Rajat, author of aer_stats:
db89ccbe52c7 ("PCI/AER: Define aer_stats structure for AER capable devices"
81aa5206f9a7 ("PCI/AER: Add sysfs attributes to provide AER stats and breakdown"]

On Thu, Dec 09, 2021 at 05:45:56PM +0800, Jiasheng Jiang wrote:
> he return value of kzalloc() needs to be checked.
> To avoid use of null pointer in case of the failure of alloc.
> 
> Fixes: db89ccbe52c7 ("PCI/AER: Define aer_stats structure for AER capable devices")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>  drivers/pci/pcie/aer.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index ec943cee5ecc..d04303edf468 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -376,6 +376,8 @@ void pci_aer_init(struct pci_dev *dev)
>  		return;
>  
>  	dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
> +	if (!dev->aer_stats)
> +		return;

Did you actually trip over a null pointer dereference, and if so,
where was it?

I think the intent here was that aer_stats is a non-essential feature,
and if we can't allocate space to keep the statistics, we can still
use the device without the stats.

I *think* all the users of dev->aer_stats check for NULL before
dereferencing it, but if you found a case that doesn't do that, we
should definitely fix it.

In a few cases (aer_stats_dev_attr, aer_stats_rootport_attr), the
check isn't obvious -- it happens in aer_stats_attrs_are_visible().
If aer_stats_attrs_are_visible() finds that aer_stats is NULL, those
sysfs attributes should not be visible, and the corresponding *_show()
functions should never be called.

>  	/*
>  	 * We save/restore PCI_ERR_UNCOR_MASK, PCI_ERR_UNCOR_SEVER,
> -- 
> 2.25.1
>

Hello

On Sat, Dec 11, 2021 at 9:54 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Rajat, author of aer_stats:
> db89ccbe52c7 ("PCI/AER: Define aer_stats structure for AER capable devices"
> 81aa5206f9a7 ("PCI/AER: Add sysfs attributes to provide AER stats and breakdown"]
>
> On Thu, Dec 09, 2021 at 05:45:56PM +0800, Jiasheng Jiang wrote:
> > he return value of kzalloc() needs to be checked.
> > To avoid use of null pointer in case of the failure of alloc.
> >
> > Fixes: db89ccbe52c7 ("PCI/AER: Define aer_stats structure for AER capable devices")
> > Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> > ---
> >  drivers/pci/pcie/aer.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index ec943cee5ecc..d04303edf468 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -376,6 +376,8 @@ void pci_aer_init(struct pci_dev *dev)
> >               return;
> >
> >       dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
> > +     if (!dev->aer_stats)
> > +             return;
>
> Did you actually trip over a null pointer dereference, and if so,
> where was it?
>
> I think the intent here was that aer_stats is a non-essential feature,
> and if we can't allocate space to keep the statistics, we can still
> use the device without the stats.

Ack. Yes, this was my intention.

Practically speaking though, if a system cannot allocate space for
aer_stats (608 bytes), I would think it is under an extreme memory
pressure so not really sure if will be able to really do anything with
the PCI device (probe the driver which may want to allocate more
memory etc). But that isn't anything that we can help with.

The current patch AFAICS looks like a no-op to me.

Thanks,

Rajat

>
> I *think* all the users of dev->aer_stats check for NULL before
> dereferencing it, but if you found a case that doesn't do that, we
> should definitely fix it.
>
> In a few cases (aer_stats_dev_attr, aer_stats_rootport_attr), the
> check isn't obvious -- it happens in aer_stats_attrs_are_visible().
> If aer_stats_attrs_are_visible() finds that aer_stats is NULL, those
> sysfs attributes should not be visible, and the corresponding *_show()
> functions should never be called.
>
> >       /*
> >        * We save/restore PCI_ERR_UNCOR_MASK, PCI_ERR_UNCOR_SEVER,
> > --
> > 2.25.1
> >

On Mon, Dec 13, 2021 at 11:57:52AM -0800, Rajat Jain wrote:
> Hello
> 
> On Sat, Dec 11, 2021 at 9:54 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Rajat, author of aer_stats:
> > db89ccbe52c7 ("PCI/AER: Define aer_stats structure for AER capable devices"
> > 81aa5206f9a7 ("PCI/AER: Add sysfs attributes to provide AER stats and breakdown"]
> >
> > On Thu, Dec 09, 2021 at 05:45:56PM +0800, Jiasheng Jiang wrote:
> > > he return value of kzalloc() needs to be checked.
> > > To avoid use of null pointer in case of the failure of alloc.
> > >
> > > Fixes: db89ccbe52c7 ("PCI/AER: Define aer_stats structure for AER capable devices")
> > > Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> > > ---
> > >  drivers/pci/pcie/aer.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > > index ec943cee5ecc..d04303edf468 100644
> > > --- a/drivers/pci/pcie/aer.c
> > > +++ b/drivers/pci/pcie/aer.c
> > > @@ -376,6 +376,8 @@ void pci_aer_init(struct pci_dev *dev)
> > >               return;
> > >
> > >       dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
> > > +     if (!dev->aer_stats)
> > > +             return;
> >
> > Did you actually trip over a null pointer dereference, and if so,
> > where was it?
> >
> > I think the intent here was that aer_stats is a non-essential feature,
> > and if we can't allocate space to keep the statistics, we can still
> > use the device without the stats.
> 
> Ack. Yes, this was my intention.
> 
> Practically speaking though, if a system cannot allocate space for
> aer_stats (608 bytes), I would think it is under an extreme memory
> pressure so not really sure if will be able to really do anything with
> the PCI device (probe the driver which may want to allocate more
> memory etc). But that isn't anything that we can help with.
> 
> The current patch AFAICS looks like a no-op to me.

Well, it's a no-op except for the fact that we should continue on to
add a save buffer and clear the AER status, so if we applied this
patch, I think an alloc failure (unlikely though it is) would mean we
would continue using the device, but the AER init for it wouldn't be
done correctly.

I think we need to drop this patch, and if there is a null pointer
dereference elsewhere, fix that.

> > I *think* all the users of dev->aer_stats check for NULL before
> > dereferencing it, but if you found a case that doesn't do that, we
> > should definitely fix it.
> >
> > In a few cases (aer_stats_dev_attr, aer_stats_rootport_attr), the
> > check isn't obvious -- it happens in aer_stats_attrs_are_visible().
> > If aer_stats_attrs_are_visible() finds that aer_stats is NULL, those
> > sysfs attributes should not be visible, and the corresponding *_show()
> > functions should never be called.
> >
> > >       /*
> > >        * We save/restore PCI_ERR_UNCOR_MASK, PCI_ERR_UNCOR_SEVER,
> > > --
> > > 2.25.1
> > >