[PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate()

[PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate()

[Gianfranco Trad]

Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate().
If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to
jump to label out with nvm_info.image_att being uninit while assigning it
to p_hwfn->nvm_info.image_att.
Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read.

Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666
Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
---
Note:
- Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ?
  
 drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
index b45efc272fdb..127943b39f61 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
@@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn)
 	}
 out:
 	/* Update hwfn's nvm_info */
-	if (nvm_info.num_images) {
+	if (nvm_info.num_images && rc != -EOPNOTSUPP) {
 		p_hwfn->nvm_info.num_images = nvm_info.num_images;
 		kfree(p_hwfn->nvm_info.image_att);
 		p_hwfn->nvm_info.image_att = nvm_info.image_att;
-- 
2.43.0

Re: [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate()

[Simon Horman]

On Wed, Dec 11, 2024 at 02:40:42PM +0100, Gianfranco Trad wrote:
> Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate().
> If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to
> jump to label out with nvm_info.image_att being uninit while assigning it
> to p_hwfn->nvm_info.image_att.
> Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read.
> 
> Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666
> Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
> ---
> Note:
> - Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ?
>   
>  drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> index b45efc272fdb..127943b39f61 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> @@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn)
>  	}
>  out:
>  	/* Update hwfn's nvm_info */
> -	if (nvm_info.num_images) {
> +	if (nvm_info.num_images && rc != -EOPNOTSUPP) {
>  		p_hwfn->nvm_info.num_images = nvm_info.num_images;
>  		kfree(p_hwfn->nvm_info.image_att);
>  		p_hwfn->nvm_info.image_att = nvm_info.image_att;

Are you sure that nvm_info.num_images can be non-zero if rc == -EOPNOTSUPP?

The cited commit state:

    Commit 43645ce03e00 ("qed: Populate nvm image attribute shadow.")
    added support for populating flash image attributes, notably
    "num_images". However, some cards were not able to return this
    information. In such cases, the driver would return EINVAL, causing the
    driver to exit.

    Add check to return EOPNOTSUPP instead of EINVAL when the card is not
    able to return these information. The caller function already handles
    EOPNOTSUPP without error.

So I would expect that nvm_info.num_images is 0.

If not, perhaps an alternate fix is to make that so, either by setting
it in qed_mcp_bist_nvm_get_num_images, or where the return value of
qed_mcp_bist_nvm_get_num_images is checked (just before the goto out).

And, in any case I think some testing is in order.

Re: [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate()

[Gianfranco Trad]

On 12/12/24 18:04, Simon Horman wrote:
> On Wed, Dec 11, 2024 at 02:40:42PM +0100, Gianfranco Trad wrote:
>> Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate().
>> If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to
>> jump to label out with nvm_info.image_att being uninit while assigning it
>> to p_hwfn->nvm_info.image_att.
>> Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read.
>>
>> Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666
>> Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
>> ---
>> Note:
>> - Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ?
>>    
>>   drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
>> index b45efc272fdb..127943b39f61 100644
>> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
>> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
>> @@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn)
>>   	}
>>   out:
>>   	/* Update hwfn's nvm_info */
>> -	if (nvm_info.num_images) {
>> +	if (nvm_info.num_images && rc != -EOPNOTSUPP) {
>>   		p_hwfn->nvm_info.num_images = nvm_info.num_images;
>>   		kfree(p_hwfn->nvm_info.image_att);
>>   		p_hwfn->nvm_info.image_att = nvm_info.image_att;
> 

Hi Simon,

> Are you sure that nvm_info.num_images can be non-zero if rc == -EOPNOTSUPP?
>

In the coverity report, the static analyzer is able to take the true 
branch on nvm_info.num_images. I didn't physically reproduce this 
logical state as I don't possess the matching hardware.

> The cited commit state:
> 
>      Commit 43645ce03e00 ("qed: Populate nvm image attribute shadow.")
>      added support for populating flash image attributes, notably
>      "num_images". However, some cards were not able to return this
>      information. In such cases, the driver would return EINVAL, causing the
>      driver to exit.
> 
>      Add check to return EOPNOTSUPP instead of EINVAL when the card is not
>      able to return these information. The caller function already handles
>      EOPNOTSUPP without error.
> 
> So I would expect that nvm_info.num_images is 0.
> 
> If not, perhaps an alternate fix is to make that so, either by setting
> it in qed_mcp_bist_nvm_get_num_images, or where the return value of
> qed_mcp_bist_nvm_get_num_images is checked (just before the goto out).
>

Makes sense, so something like this I suppose:

--- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
@@ -3301,8 +3301,10 @@ int qed_mcp_bist_nvm_get_num_images(struct 
qed_hwfn *p_hwfn,
   	if (rc)
   		return rc;

-	if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED))
+	if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED)) {
+		*num_images = 0;
   		rc = -EOPNOTSUPP;
+	}

Or the second option you stated.

> And, in any case I think some testing is in order.

I strongly agree. Let me know if I can help more with this.

Thanks for your time,
--Gian

Re: [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate()

[Simon Horman]

On Fri, Dec 13, 2024 at 01:13:12PM +0100, Gianfranco Trad wrote:
> On 12/12/24 18:04, Simon Horman wrote:
> > On Wed, Dec 11, 2024 at 02:40:42PM +0100, Gianfranco Trad wrote:
> > > Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate().
> > > If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to
> > > jump to label out with nvm_info.image_att being uninit while assigning it
> > > to p_hwfn->nvm_info.image_att.
> > > Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read.
> > > 
> > > Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666
> > > Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
> > > ---
> > > Note:
> > > - Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ?
> > >   drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> > > index b45efc272fdb..127943b39f61 100644
> > > --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> > > +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> > > @@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn)
> > >   	}
> > >   out:
> > >   	/* Update hwfn's nvm_info */
> > > -	if (nvm_info.num_images) {
> > > +	if (nvm_info.num_images && rc != -EOPNOTSUPP) {
> > >   		p_hwfn->nvm_info.num_images = nvm_info.num_images;
> > >   		kfree(p_hwfn->nvm_info.image_att);
> > >   		p_hwfn->nvm_info.image_att = nvm_info.image_att;
> > 
> 
> Hi Simon,
> 
> > Are you sure that nvm_info.num_images can be non-zero if rc == -EOPNOTSUPP?
> > 
> 
> In the coverity report, the static analyzer is able to take the true branch
> on nvm_info.num_images. I didn't physically reproduce this logical state as
> I don't possess the matching hardware.
> 
> > The cited commit state:
> > 
> >      Commit 43645ce03e00 ("qed: Populate nvm image attribute shadow.")
> >      added support for populating flash image attributes, notably
> >      "num_images". However, some cards were not able to return this
> >      information. In such cases, the driver would return EINVAL, causing the
> >      driver to exit.
> > 
> >      Add check to return EOPNOTSUPP instead of EINVAL when the card is not
> >      able to return these information. The caller function already handles
> >      EOPNOTSUPP without error.
> > 
> > So I would expect that nvm_info.num_images is 0.
> > 
> > If not, perhaps an alternate fix is to make that so, either by setting
> > it in qed_mcp_bist_nvm_get_num_images, or where the return value of
> > qed_mcp_bist_nvm_get_num_images is checked (just before the goto out).
> > 
> 
> Makes sense, so something like this I suppose:
> 
> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
> @@ -3301,8 +3301,10 @@ int qed_mcp_bist_nvm_get_num_images(struct qed_hwfn
> *p_hwfn,
>   	if (rc)
>   		return rc;
> 
> -	if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED))
> +	if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED)) {
> +		*num_images = 0;
>   		rc = -EOPNOTSUPP;
> +	}
> 
> Or the second option you stated.

Yes, that is what I was thinking.
But as it is a side effect, and thus hidden slightly,
on reflection perhaps the second option is better. IDK.

> > And, in any case I think some testing is in order.
> 
> I strongly agree. Let me know if I can help more with this.
> 
> Thanks for your time,
> --Gian
> 

Re: [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate()

[Gianfranco Trad]

On 13/12/24 14:31, Simon Horman wrote:
> On Fri, Dec 13, 2024 at 01:13:12PM +0100, Gianfranco Trad wrote:
>> On 12/12/24 18:04, Simon Horman wrote:
>>> On Wed, Dec 11, 2024 at 02:40:42PM +0100, Gianfranco Trad wrote:
>>>> Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate().
>>>> If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to
>>>> jump to label out with nvm_info.image_att being uninit while assigning it
>>>> to p_hwfn->nvm_info.image_att.
>>>> Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read.
>>>>
>>>> Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666
>>>> Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
>>>> ---
>>>> Note:
>>>> - Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ?
>>>>    drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
>>>> index b45efc272fdb..127943b39f61 100644
>>>> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
>>>> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
>>>> @@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn)
>>>>    	}
>>>>    out:
>>>>    	/* Update hwfn's nvm_info */
>>>> -	if (nvm_info.num_images) {
>>>> +	if (nvm_info.num_images && rc != -EOPNOTSUPP) {
>>>>    		p_hwfn->nvm_info.num_images = nvm_info.num_images;
>>>>    		kfree(p_hwfn->nvm_info.image_att);
>>>>    		p_hwfn->nvm_info.image_att = nvm_info.image_att;
>>>
>>
>> Hi Simon,
>>
>>> Are you sure that nvm_info.num_images can be non-zero if rc == -EOPNOTSUPP?
>>>
>>
>> In the coverity report, the static analyzer is able to take the true branch
>> on nvm_info.num_images. I didn't physically reproduce this logical state as
>> I don't possess the matching hardware.
>>
>>> The cited commit state:
>>>
>>>       Commit 43645ce03e00 ("qed: Populate nvm image attribute shadow.")
>>>       added support for populating flash image attributes, notably
>>>       "num_images". However, some cards were not able to return this
>>>       information. In such cases, the driver would return EINVAL, causing the
>>>       driver to exit.
>>>
>>>       Add check to return EOPNOTSUPP instead of EINVAL when the card is not
>>>       able to return these information. The caller function already handles
>>>       EOPNOTSUPP without error.
>>>
>>> So I would expect that nvm_info.num_images is 0.
>>>
>>> If not, perhaps an alternate fix is to make that so, either by setting
>>> it in qed_mcp_bist_nvm_get_num_images, or where the return value of
>>> qed_mcp_bist_nvm_get_num_images is checked (just before the goto out).
>>>
>>
>> Makes sense, so something like this I suppose:
>>
>> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
>> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
>> @@ -3301,8 +3301,10 @@ int qed_mcp_bist_nvm_get_num_images(struct qed_hwfn
>> *p_hwfn,
>>    	if (rc)
>>    		return rc;
>>
>> -	if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED))
>> +	if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED)) {
>> +		*num_images = 0;
>>    		rc = -EOPNOTSUPP;
>> +	}
>>
>> Or the second option you stated.
> 
> Yes, that is what I was thinking.
> But as it is a side effect, and thus hidden slightly,
> on reflection perhaps the second option is better. IDK.
> 

Got it. Will send a v2 accordingly.

Thanks for your time,
--Gian

>>> And, in any case I think some testing is in order.
>>
>> I strongly agree. Let me know if I can help more with this.
>>
>> Thanks for your time,
>> --Gian
>>