* [PATCH] crypto: octeontx2: fix potential null pointer access
@ 2022-05-27 7:57 Shijith Thotton
2022-05-27 8:19 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Shijith Thotton @ 2022-05-27 7:57 UTC (permalink / raw)
To: Arnaud Ebalard, Herbert Xu, Boris Brezillon
Cc: Shijith Thotton, linux-crypto, jerinj, sgoutham, Srujana Challa,
David S. Miller, Harman Kalra, Yang Yingliang, Dan Carpenter,
Kees Cook, Jiapeng Chong, open list
Added missing checks to avoid null pointer dereference.
The patch fixes below issues reported by klocwork tool:
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 <sthotton@marvell.com>
---
.../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);
+ 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, ':')) {
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)
@@ -1617,7 +1621,11 @@ int otx2_cpt_dl_custom_egrp_create(struct otx2_cptpf_dev *cptpf,
} else if (!strncasecmp(val, "ae", 2) && strchr(val, ':')) {
if (has_ae || 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)
@@ -1629,7 +1637,11 @@ int otx2_cpt_dl_custom_egrp_create(struct otx2_cptpf_dev *cptpf,
} else if (!strncasecmp(val, "ie", 2) && strchr(val, ':')) {
if (has_ie || 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)
diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c b/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
index 3411e664cf50..9249ec1783bc 100644
--- a/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
+++ b/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
@@ -334,6 +334,7 @@ static int otx2_cptvf_probe(struct pci_dev *pdev,
{
struct device *dev = &pdev->dev;
struct otx2_cptvf_dev *cptvf;
+ void __iomem * const *iomap;
int ret;
cptvf = devm_kzalloc(dev, sizeof(*cptvf), GFP_KERNEL);
@@ -362,7 +363,13 @@ static int otx2_cptvf_probe(struct pci_dev *pdev,
pci_set_drvdata(pdev, cptvf);
cptvf->pdev = pdev;
- cptvf->reg_base = pcim_iomap_table(pdev)[PCI_PF_REG_BAR_NUM];
+ iomap = pcim_iomap_table(pdev);
+ if (!iomap) {
+ dev_err(dev, "Failed to get iomap table\n");
+ ret = -ENODEV;
+ goto clear_drvdata;
+ }
+ cptvf->reg_base = iomap[PCI_PF_REG_BAR_NUM];
otx2_cpt_set_hw_caps(pdev, &cptvf->cap_flag);
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] crypto: octeontx2: fix potential null pointer access
2022-05-27 7:57 [PATCH] crypto: octeontx2: fix potential null pointer access Shijith Thotton
@ 2022-05-27 8:19 ` Dan Carpenter
2022-05-27 9:40 ` [EXT] " Shijith Thotton
2022-05-27 8:23 ` Dan Carpenter
2022-06-01 8:08 ` [PATCH v2] " Shijith Thotton
2 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2022-05-27 8:19 UTC (permalink / raw)
To: Shijith Thotton
Cc: Arnaud Ebalard, Herbert Xu, Boris Brezillon, linux-crypto,
jerinj, sgoutham, Srujana Challa, David S. Miller, Harman Kalra,
Yang Yingliang, Kees Cook, Jiapeng Chong, open list
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 <sthotton@marvell.com>
> ---
> .../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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] crypto: octeontx2: fix potential null pointer access
2022-05-27 7:57 [PATCH] crypto: octeontx2: fix potential null pointer access Shijith Thotton
2022-05-27 8:19 ` Dan Carpenter
@ 2022-05-27 8:23 ` Dan Carpenter
2022-05-27 9:42 ` [EXT] " Shijith Thotton
2022-06-01 8:08 ` [PATCH v2] " Shijith Thotton
2 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2022-05-27 8:23 UTC (permalink / raw)
To: Shijith Thotton
Cc: Arnaud Ebalard, Herbert Xu, Boris Brezillon, linux-crypto,
jerinj, sgoutham, Srujana Challa, David S. Miller, Harman Kalra,
Yang Yingliang, Kees Cook, Jiapeng Chong, open list
On Fri, May 27, 2022 at 01:27:56PM +0530, Shijith Thotton wrote:
> 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, ':')) {
> 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;
The check is not needed here, but if it were then the better way to
write this would be:
tmp = strsep(&val, ":");
if (!tmp)
goto err_print;
tmp = strim(tmp);
Always to error handling, not success handling. checkpatch.pl --strict
will complain about the != NULL.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [EXT] Re: [PATCH] crypto: octeontx2: fix potential null pointer access
2022-05-27 8:19 ` Dan Carpenter
@ 2022-05-27 9:40 ` Shijith Thotton
2022-05-27 10:04 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: Shijith Thotton @ 2022-05-27 9:40 UTC (permalink / raw)
To: Dan Carpenter
Cc: Arnaud Ebalard, Herbert Xu, Boris Brezillon, linux-crypto,
Jerin Jacob Kollanukkaran, Sunil Kovvuri Goutham, Srujana Challa,
David S. Miller, Harman Kalra, Yang Yingliang, Kees Cook,
Jiapeng Chong, open list
>> 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 <sthotton@marvell.com>
>> ---
>> .../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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [EXT] Re: [PATCH] crypto: octeontx2: fix potential null pointer access
2022-05-27 8:23 ` Dan Carpenter
@ 2022-05-27 9:42 ` Shijith Thotton
0 siblings, 0 replies; 9+ messages in thread
From: Shijith Thotton @ 2022-05-27 9:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: Arnaud Ebalard, Herbert Xu, Boris Brezillon, linux-crypto,
Jerin Jacob Kollanukkaran, Sunil Kovvuri Goutham, Srujana Challa,
David S. Miller, Harman Kalra, Yang Yingliang, Kees Cook,
Jiapeng Chong, open list
>> 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, ':')) {
>> 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;
>
>The check is not needed here, but if it were then the better way to
>write this would be:
>
> tmp = strsep(&val, ":");
> if (!tmp)
> goto err_print;
> tmp = strim(tmp);
>
>Always to error handling, not success handling. checkpatch.pl --strict
>will complain about the != NULL.
>
Will change the check as mentioned.
Thanks,
Shijith
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [EXT] Re: [PATCH] crypto: octeontx2: fix potential null pointer access
2022-05-27 9:40 ` [EXT] " Shijith Thotton
@ 2022-05-27 10:04 ` Dan Carpenter
2022-05-27 11:14 ` Shijith Thotton
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2022-05-27 10:04 UTC (permalink / raw)
To: Shijith Thotton
Cc: Arnaud Ebalard, Herbert Xu, Boris Brezillon, linux-crypto,
Jerin Jacob Kollanukkaran, Sunil Kovvuri Goutham, Srujana Challa,
David S. Miller, Harman Kalra, Yang Yingliang, Kees Cook,
Jiapeng Chong, open list
On Fri, May 27, 2022 at 09:40:16AM +0000, Shijith Thotton wrote:
> >> @@ -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;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [EXT] Re: [PATCH] crypto: octeontx2: fix potential null pointer access
2022-05-27 10:04 ` Dan Carpenter
@ 2022-05-27 11:14 ` Shijith Thotton
0 siblings, 0 replies; 9+ messages in thread
From: Shijith Thotton @ 2022-05-27 11:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: Arnaud Ebalard, Herbert Xu, Boris Brezillon, linux-crypto,
Jerin Jacob Kollanukkaran, Sunil Kovvuri Goutham, Srujana Challa,
David S. Miller, Harman Kalra, Yang Yingliang, Kees Cook,
Jiapeng Chong, open list
>> >> @@ -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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] crypto: octeontx2: fix potential null pointer access
2022-05-27 7:57 [PATCH] crypto: octeontx2: fix potential null pointer access Shijith Thotton
2022-05-27 8:19 ` Dan Carpenter
2022-05-27 8:23 ` Dan Carpenter
@ 2022-06-01 8:08 ` Shijith Thotton
2022-06-10 9:16 ` Herbert Xu
2 siblings, 1 reply; 9+ messages in thread
From: Shijith Thotton @ 2022-06-01 8:08 UTC (permalink / raw)
To: Arnaud Ebalard, Herbert Xu, Boris Brezillon, Dan Carpenter
Cc: Shijith Thotton, linux-crypto, jerinj, sgoutham, Srujana Challa,
David S. Miller, Jiapeng Chong, Kees Cook, open list
Added missing checks to avoid null pointer dereference.
The patch fixes below issue reported by klocwork tool:
. 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 <sthotton@marvell.com>
---
v2:
* Removed unwanted check for pcim_iomap_table.
.../crypto/marvell/octeontx2/otx2_cptpf_ucode.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c b/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
index 9cba2f714c7e..080cbfa093ec 100644
--- a/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
+++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf_ucode.c
@@ -1605,7 +1605,10 @@ int otx2_cpt_dl_custom_egrp_create(struct otx2_cptpf_dev *cptpf,
if (!strncasecmp(val, "se", 2) && strchr(val, ':')) {
if (has_se || ucode_idx)
goto err_print;
- tmp = strim(strsep(&val, ":"));
+ tmp = strsep(&val, ":");
+ if (!tmp)
+ goto err_print;
+ tmp = strim(tmp);
if (!val)
goto err_print;
if (strlen(tmp) != 2)
@@ -1617,7 +1620,10 @@ int otx2_cpt_dl_custom_egrp_create(struct otx2_cptpf_dev *cptpf,
} else if (!strncasecmp(val, "ae", 2) && strchr(val, ':')) {
if (has_ae || ucode_idx)
goto err_print;
- tmp = strim(strsep(&val, ":"));
+ tmp = strsep(&val, ":");
+ if (!tmp)
+ goto err_print;
+ tmp = strim(tmp);
if (!val)
goto err_print;
if (strlen(tmp) != 2)
@@ -1629,7 +1635,10 @@ int otx2_cpt_dl_custom_egrp_create(struct otx2_cptpf_dev *cptpf,
} else if (!strncasecmp(val, "ie", 2) && strchr(val, ':')) {
if (has_ie || ucode_idx)
goto err_print;
- tmp = strim(strsep(&val, ":"));
+ tmp = strsep(&val, ":");
+ if (!tmp)
+ goto err_print;
+ tmp = strim(tmp);
if (!val)
goto err_print;
if (strlen(tmp) != 2)
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] crypto: octeontx2: fix potential null pointer access
2022-06-01 8:08 ` [PATCH v2] " Shijith Thotton
@ 2022-06-10 9:16 ` Herbert Xu
0 siblings, 0 replies; 9+ messages in thread
From: Herbert Xu @ 2022-06-10 9:16 UTC (permalink / raw)
To: Shijith Thotton
Cc: Arnaud Ebalard, Boris Brezillon, Dan Carpenter, linux-crypto,
jerinj, sgoutham, Srujana Challa, David S. Miller, Jiapeng Chong,
Kees Cook, open list
On Wed, Jun 01, 2022 at 01:38:59PM +0530, Shijith Thotton wrote:
> Added missing checks to avoid null pointer dereference.
>
> The patch fixes below issue reported by klocwork tool:
> . 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 <sthotton@marvell.com>
> ---
> v2:
> * Removed unwanted check for pcim_iomap_table.
>
> .../crypto/marvell/octeontx2/otx2_cptpf_ucode.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-06-10 9:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-27 7:57 [PATCH] crypto: octeontx2: fix potential null pointer access Shijith Thotton
2022-05-27 8:19 ` Dan Carpenter
2022-05-27 9:40 ` [EXT] " Shijith Thotton
2022-05-27 10:04 ` Dan Carpenter
2022-05-27 11:14 ` Shijith Thotton
2022-05-27 8:23 ` Dan Carpenter
2022-05-27 9:42 ` [EXT] " Shijith Thotton
2022-06-01 8:08 ` [PATCH v2] " Shijith Thotton
2022-06-10 9:16 ` Herbert Xu
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).