RE: [EXT] Re: [PATCH] crypto: octeontx2: fix potential null pointer access

From: Shijith Thotton <sthotton@marvell.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Arnaud Ebalard <arno@natisbad.org>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Boris Brezillon <bbrezillon@kernel.org>,
	"linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
	Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
	Sunil Kovvuri Goutham <sgoutham@marvell.com>,
	Srujana Challa <schalla@marvell.com>,
	"David S. Miller" <davem@davemloft.net>,
	Harman Kalra <hkalra@marvell.com>,
	Yang Yingliang <yangyingliang@huawei.com>,
	Kees Cook <keescook@chromium.org>,
	Jiapeng Chong <jiapeng.chong@linux.alibaba.com>,
	open list <linux-kernel@vger.kernel.org>
Subject: RE: [EXT] Re: [PATCH] crypto: octeontx2: fix potential null pointer access
Date: Fri, 27 May 2022 11:14:11 +0000	[thread overview]
Message-ID: <PH0PR18MB4425583674C4EDA9F8266545D9D89@PH0PR18MB4425.namprd18.prod.outlook.com> (raw)
In-Reply-To: <20220527100402.GQ2168@kadam>

>> >> @@ -731,7 +732,13 @@ static int otx2_cptpf_probe(struct pci_dev *pdev,
>> >>  	pci_set_drvdata(pdev, cptpf);
>> >>  	cptpf->pdev = pdev;
>> >>
>> >> -	cptpf->reg_base = pcim_iomap_table(pdev)[PCI_PF_REG_BAR_NUM];
>> >> +	iomap = pcim_iomap_table(pdev);
>> >
>> >I don't know if a check is required here or not...  The comments to
>> >pcim_iomap_table() say it is, "guaranteed to succeed once  allocated."
>> >
>>
>> Will keep the check just to be safe, as allocation/kmalloc could fail.
>
>No, it cannot fail.
>
>I don't care if you add pointless NULL checks to make the static checker
>happy, but it's important to understand what the code is doing.
>
>drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
>   701  static int otx2_cptpf_probe(struct pci_dev *pdev,
>   702                              const struct pci_device_id *ent)
>   703  {
>   704          struct device *dev = &pdev->dev;
>   705          struct otx2_cptpf_dev *cptpf;
>   706          int err;
>   707
>   708          cptpf = devm_kzalloc(dev, sizeof(*cptpf), GFP_KERNEL);
>   709          if (!cptpf)
>   710                  return -ENOMEM;
>   711
>   712          err = pcim_enable_device(pdev);
>   713          if (err) {
>   714                  dev_err(dev, "Failed to enable PCI device\n");
>   715                  goto clear_drvdata;
>   716          }
>   717
>   718          err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48));
>   719          if (err) {
>   720                  dev_err(dev, "Unable to get usable DMA configuration\n");
>   721                  goto clear_drvdata;
>   722          }
>   723          /* Map PF's configuration registers */
>   724          err = pcim_iomap_regions_request_all(pdev, 1 <<
>PCI_PF_REG_BAR_NUM,
>   725                                               OTX2_CPT_DRV_NAME);
>
>The pcim_iomap_table() is allocated here inside the pcim_iomap_regions()
>function.
>
>   726          if (err) {
>   727                  dev_err(dev, "Couldn't get PCI resources 0x%x\n", err);
>   728                  goto clear_drvdata;
>   729          }
>   730          pci_set_master(pdev);
>   731          pci_set_drvdata(pdev, cptpf);
>   732          cptpf->pdev = pdev;
>   733
>   734          cptpf->reg_base = pcim_iomap_table(pdev)[PCI_PF_REG_BAR_NUM];
>
>It cannot fail here.  It is not allocated here.  We just look it up and
>use it.
>
>   735
>   736          /* Check if AF driver is up, otherwise defer probe */
>   737          err = cpt_is_pf_usable(cptpf);
>   738          if (err)
>   739                  goto clear_drvdata;
>

Thanks Dan, I got it now. I will remove the check from v2.