On Fri, May 27, 2022 at 01:27:56PM +0530, Shijith Thotton wrote:
> Added missing checks to avoid null pointer dereference.
>
> The patch fixes below issues reported by klocwork tool:
Don't fix false positives to make a tool happy. Fix the tool. (Unless
the patch makes the code simpler, then it's fine).
> 1. Pointer 'pcim_iomap_table(pdev)' returned from call to function
> 'pcim_iomap_table' at line 365 may be NULL and will be dereferenced
> at line 365 in otx2_cptvf_main.c. Also there is a similar error on
> line 734 in otx2_cptpf_main.c.
> 2. Pointer 'strsep( &val, ":" )' returned from call to function 'strsep'
> at line 1608 may be NULL and will be dereferenced at line 1608. Also
> there are 2 similar errors on lines 1620, 1632 in otx2_cptpf_ucode.c.
>
> Signed-off-by: Shijith Thotton <[email protected]>
> ---
> .../crypto/marvell/octeontx2/otx2_cptpf_main.c | 9 ++++++++-
> .../marvell/octeontx2/otx2_cptpf_ucode.c | 18 +++++++++++++++---
> .../crypto/marvell/octeontx2/otx2_cptvf_main.c | 9 ++++++++-
> 3 files changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
> index a402ccfac557..ae57cee424f0 100644
> --- a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
> +++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
> @@ -703,6 +703,7 @@ static int otx2_cptpf_probe(struct pci_dev *pdev,
> {
> struct device *dev = &pdev->dev;
> struct otx2_cptpf_dev *cptpf;
> + void __iomem * const *iomap;
> int err;
>
> cptpf = devm_kzalloc(dev, sizeof(*cptpf), GFP_KERNEL);
> @@ -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."
> + if (!iomap) {
> + dev_err(dev, "Failed to get iomap table\n");
> + err = -ENODEV;
> + goto clear_drvdata;
> + }
> + cptpf->reg_base = iomap[PCI_PF_REG_BAR_NUM];
>
> /* Check if AF driver is up, otherwise defer probe */
> err = cpt_is_pf_usable(cptpf);
> diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c b/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
> index 9cba2f714c7e..b91401929fc6 100644
> --- a/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
> +++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
> @@ -1605,7 +1605,11 @@ int otx2_cpt_dl_custom_egrp_create(struct otx2_cptpf_dev *cptpf,
> if (!strncasecmp(val, "se", 2) && strchr(val, ':')) {
^^^^^^^^^^^^^^^^
We know it can't be NULL.
> if (has_se || ucode_idx)
> goto err_print;
> - tmp = strim(strsep(&val, ":"));
> + tmp = strsep(&val, ":");
> + if (tmp != NULL)
> + tmp = strim(tmp);
> + else
> + goto err_print;
> if (!val)
> goto err_print;
> if (strlen(tmp) != 2)
The rest is all the same. Likely or definitely false positives.
regards,
dan carpenter
>> Added missing checks to avoid null pointer dereference.
>>
>> The patch fixes below issues reported by klocwork tool:
>
>Don't fix false positives to make a tool happy. Fix the tool. (Unless
>the patch makes the code simpler, then it's fine).
>
>> 1. Pointer 'pcim_iomap_table(pdev)' returned from call to function
>> 'pcim_iomap_table' at line 365 may be NULL and will be dereferenced
>> at line 365 in otx2_cptvf_main.c. Also there is a similar error on
>> line 734 in otx2_cptpf_main.c.
>> 2. Pointer 'strsep( &val, ":" )' returned from call to function 'strsep'
>> at line 1608 may be NULL and will be dereferenced at line 1608. Also
>> there are 2 similar errors on lines 1620, 1632 in otx2_cptpf_ucode.c.
>>
>> Signed-off-by: Shijith Thotton <[email protected]>
>> ---
>> .../crypto/marvell/octeontx2/otx2_cptpf_main.c | 9 ++++++++-
>> .../marvell/octeontx2/otx2_cptpf_ucode.c | 18 +++++++++++++++---
>> .../crypto/marvell/octeontx2/otx2_cptvf_main.c | 9 ++++++++-
>> 3 files changed, 31 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
>b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
>> index a402ccfac557..ae57cee424f0 100644
>> --- a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
>> +++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
>> @@ -703,6 +703,7 @@ static int otx2_cptpf_probe(struct pci_dev *pdev,
>> {
>> struct device *dev = &pdev->dev;
>> struct otx2_cptpf_dev *cptpf;
>> + void __iomem * const *iomap;
>> int err;
>>
>> cptpf = devm_kzalloc(dev, sizeof(*cptpf), GFP_KERNEL);
>> @@ -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.
>> + if (!iomap) {
>> + dev_err(dev, "Failed to get iomap table\n");
>> + err = -ENODEV;
>> + goto clear_drvdata;
>> + }
>> + cptpf->reg_base = iomap[PCI_PF_REG_BAR_NUM];
>>
>> /* Check if AF driver is up, otherwise defer probe */
>> err = cpt_is_pf_usable(cptpf);
>> diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
>b/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
>> index 9cba2f714c7e..b91401929fc6 100644
>> --- a/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
>> +++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
>> @@ -1605,7 +1605,11 @@ int otx2_cpt_dl_custom_egrp_create(struct
>otx2_cptpf_dev *cptpf,
>> if (!strncasecmp(val, "se", 2) && strchr(val, ':')) {
> ^^^^^^^^^^^^^^^^
>We know it can't be NULL.
>
>> if (has_se || ucode_idx)
>> goto err_print;
>> - tmp = strim(strsep(&val, ":"));
>> + tmp = strsep(&val, ":");
>> + if (tmp != NULL)
>> + tmp = strim(tmp);
>> + else
>> + goto err_print;
>> if (!val)
>> goto err_print;
>> if (strlen(tmp) != 2)
>
>The rest is all the same. Likely or definitely false positives.
>
>regards,
>dan carpenter