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

[Simon Horman <horms@kernel.org>]

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.