linux-kernel - Re: [PATCH net-next v4 01/10] net: sunhme: Fix uninitialized return code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZB6t/64NoGejdxUG@corigine.com>
Date:   Sat, 25 Mar 2023 09:17:03 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Sean Anderson <seanga2@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>,
        Dan Carpenter <error27@...il.com>
Subject: Re: [PATCH net-next v4 01/10] net: sunhme: Fix uninitialized return
 code

Hi Sean,

On Fri, Mar 24, 2023 at 01:51:27PM -0400, Sean Anderson wrote:
> Fix an uninitialized return code if we never found a qfe slot. It would be
> a bug if we ever got into this situation, but it's good to return something
> tracable.
> 
> Fixes: acb3f35f920b ("sunhme: forward the error code from pci_enable_device()")

I think it might be,

Fixes: 5b3dc6dda6b1 ("sunhme: Regularize probe errors")

> Reported-by: kernel test robot <lkp@...el.com>
> Reported-by: Dan Carpenter <error27@...il.com>

Checkpatch requests a Link tag after Reported-by tags.

Link: https://lore.kernel.org/oe-kbuild/20230222135715.hjXBN9H5dr7nCnI_Ye2s5H--HsnWom4o9AMScThhDRM@z/T/

> Signed-off-by: Sean Anderson <seanga2@...il.com>
> ---
> 
> Changes in v4:
> - Move this fix to its own commit
> 
>  drivers/net/ethernet/sun/sunhme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index b0c7ab74a82e..7cf8210ebbec 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c
> @@ -2834,7 +2834,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
>  	int i, qfe_slot = -1;
>  	char prom_name[64];
>  	u8 addr[ETH_ALEN];
> -	int err;
> +	int err = -ENODEV;
>  
>  	/* Now make sure pci_dev cookie is there. */
>  #ifdef CONFIG_SPARC

Unfortunately I don't think this is the right fix,
and indeed smatch still complains with it applied.

The reason is that a few lines further down there is:

        err = pcim_enable_device(pdev);
        if (err)
                goto err_out;

Which overrides the initialisation of err.
Before getting to the line that smatch highlights, correctly as far
as I can tell, as having a missing error code.

			if (qfe_slot == 4)
				goto err_out;

As err_out simply calls 'return err' one could just return here.
And perhaps that is a nice cleanup to roll out at some point.
But to be in keeping with the style of the function, as a minimal fix,
I think the following might be appropriate.

Note, with this applied err doesn't need to be initialised at the top of
the function (as far as my invocation of smatch is concerned).

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index b0c7ab74a82e..d6df778a0052 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -2863,8 +2863,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 			if (!qp->happy_meals[qfe_slot])
 				break;
 
-		if (qfe_slot == 4)
+		if (qfe_slot == 4) {
+			err = -ENODEV;
 			goto err_out;
+		}
 	}
 
 	dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));