* [PATCH] PCI/AER: potential dereference of null pointer @ 2021-12-09 9:45 Jiasheng Jiang 2021-12-11 17:54 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Jiasheng Jiang @ 2021-12-09 9:45 UTC (permalink / raw) To: ruscur, oohall, bhelgaas Cc: linuxppc-dev, linux-pci, linux-kernel, Jiasheng Jiang 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; /* * We save/restore PCI_ERR_UNCOR_MASK, PCI_ERR_UNCOR_SEVER, -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI/AER: potential dereference of null pointer 2021-12-09 9:45 [PATCH] PCI/AER: potential dereference of null pointer Jiasheng Jiang @ 2021-12-11 17:54 ` Bjorn Helgaas 2021-12-13 19:57 ` Rajat Jain 0 siblings, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2021-12-11 17:54 UTC (permalink / raw) To: Jiasheng Jiang Cc: ruscur, oohall, bhelgaas, linuxppc-dev, linux-pci, linux-kernel, Rajat Jain [+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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI/AER: potential dereference of null pointer 2021-12-11 17:54 ` Bjorn Helgaas @ 2021-12-13 19:57 ` Rajat Jain 2021-12-13 20:07 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Rajat Jain @ 2021-12-13 19:57 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jiasheng Jiang, ruscur, oohall, bhelgaas, linuxppc-dev, linux-pci, linux-kernel 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI/AER: potential dereference of null pointer 2021-12-13 19:57 ` Rajat Jain @ 2021-12-13 20:07 ` Bjorn Helgaas 0 siblings, 0 replies; 4+ messages in thread From: Bjorn Helgaas @ 2021-12-13 20:07 UTC (permalink / raw) To: Rajat Jain Cc: Jiasheng Jiang, ruscur, oohall, bhelgaas, linuxppc-dev, linux-pci, linux-kernel 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 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-13 20:07 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-09 9:45 [PATCH] PCI/AER: potential dereference of null pointer Jiasheng Jiang 2021-12-11 17:54 ` Bjorn Helgaas 2021-12-13 19:57 ` Rajat Jain 2021-12-13 20:07 ` Bjorn Helgaas
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).