Re: [bug report] PCI: endpoint: Remove "core_init_notifier" flag

[Niklas Cassel <cassel@kernel.org>]

On Wed, Apr 17, 2024 at 03:47:48PM +0300, Dan Carpenter wrote:
> Hello Manivannan Sadhasivam,
> 
> Commit a01e7214bef9 ("PCI: endpoint: Remove "core_init_notifier"
> flag") from Mar 27, 2024 (linux-next), leads to the following Smatch
> static checker warning:
> 
> 	drivers/pci/endpoint/functions/pci-epf-test.c:784 pci_epf_test_core_init()
> 	error: we previously assumed 'epc_features' could be null (see line 747)
> 
> drivers/pci/endpoint/functions/pci-epf-test.c
>     734 static int pci_epf_test_core_init(struct pci_epf *epf)
>     735 {
>     736         struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>     737         struct pci_epf_header *header = epf->header;
>     738         const struct pci_epc_features *epc_features;
>     739         struct pci_epc *epc = epf->epc;
>     740         struct device *dev = &epf->dev;
>     741         bool linkup_notifier = false;
>     742         bool msix_capable = false;
>     743         bool msi_capable = true;
>     744         int ret;

We check pci_epc_get_features() in ->bind(), which comes before ->core_init()
so in practice, this shouldn't be able to be NULL here.


>     745 
>     746         epc_features = pci_epc_get_features(epc, epf->func_no, epf->vfunc_no);
>     747         if (epc_features) {
>                     ^^^^^^^^^^^^
> epc_features can be NULL

We should probably just chge this to:

"""
if (!epc_features)
	return some_error;

msix_capable = epc_features->msix_capable;
msi_capable = epc_features->msi_capable;
"""

Such that we don't need another check further down for linkup_notifier.


Kind regards,
Niklas

> 
>     748                 msix_capable = epc_features->msix_capable;
>     749                 msi_capable = epc_features->msi_capable;
>     750         }
>     751 
>     752         if (epf->vfunc_no <= 1) {
>     753                 ret = pci_epc_write_header(epc, epf->func_no, epf->vfunc_no, header);
>     754                 if (ret) {
>     755                         dev_err(dev, "Configuration header write failed\n");
>     756                         return ret;
>     757                 }
>     758         }
>     759 
>     760         ret = pci_epf_test_set_bar(epf);
>     761         if (ret)
>     762                 return ret;
>     763 
>     764         if (msi_capable) {
>     765                 ret = pci_epc_set_msi(epc, epf->func_no, epf->vfunc_no,
>     766                                       epf->msi_interrupts);
>     767                 if (ret) {
>     768                         dev_err(dev, "MSI configuration failed\n");
>     769                         return ret;
>     770                 }
>     771         }
>     772 
>     773         if (msix_capable) {
>     774                 ret = pci_epc_set_msix(epc, epf->func_no, epf->vfunc_no,
>     775                                        epf->msix_interrupts,
>     776                                        epf_test->test_reg_bar,
>     777                                        epf_test->msix_table_offset);
>     778                 if (ret) {
>     779                         dev_err(dev, "MSI-X configuration failed\n");
>     780                         return ret;
>     781                 }
>     782         }
>     783 
> --> 784         linkup_notifier = epc_features->linkup_notifier;
>                                   ^^^^^^^^^^^^^^
> Unchecked dereference.
> 
>     785         if (!linkup_notifier)
>     786                 queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work);
>     787 
>     788         return 0;
>     789 }
> 
> regards,
> dan carpenter