linux-kernel - Re: [PATCH][next] net: dsa: sja1105: fix comparisons against uninitialized status fields

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: <a8931feb-c11a-3833-0a14-2585e70c9114@canonical.com>
Date:   Tue, 7 May 2019 10:29:07 +0100
From:   Colin Ian King <colin.king@...onical.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Vladimir Oltean <olteanv@...il.com>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH][next] net: dsa: sja1105: fix comparisons against
 uninitialized status fields

On 07/05/2019 10:20, Dan Carpenter wrote:
> On Tue, May 07, 2019 at 09:44:58AM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@...onical.com>
>>
>> The call to sja1105_status_get to set various fields in the status
>> structure can potentially be skipped in a while-loop because of a couple
>> of prior continuation jump paths. This can potientially lead to checking
>> be checking against an uninitialized fields in the structure which may
>> lead to unexpected results.  Fix this by ensuring all the fields in status
>> are initialized to zero to be safe.
>>
>> Addresses-Coverity: ("Uninitialized scalar variable")
>> Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch")
>> Signed-off-by: Colin Ian King <colin.king@...onical.com>
>> ---
>>  drivers/net/dsa/sja1105/sja1105_spi.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
>> index 244a94ccfc18..76f6a51e10d9 100644
>> --- a/drivers/net/dsa/sja1105/sja1105_spi.c
>> +++ b/drivers/net/dsa/sja1105/sja1105_spi.c
>> @@ -394,7 +394,7 @@ int sja1105_static_config_upload(struct sja1105_private *priv)
>>  	struct sja1105_static_config *config = &priv->static_config;
>>  	const struct sja1105_regs *regs = priv->info->regs;
>>  	struct device *dev = &priv->spidev->dev;
>> -	struct sja1105_status status;
>> +	struct sja1105_status status = {};
> 
> The exit condition isn't right.  It should continue if ret is negative
> or the CRC stuff is invalid but right now it's ignoring ret.  It would
> be better could just add a break statement at the very end and remove
> the status checks.  Like so:
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
> index 244a94ccfc18..3af3b0f3cc44 100644
> --- a/drivers/net/dsa/sja1105/sja1105_spi.c
> +++ b/drivers/net/dsa/sja1105/sja1105_spi.c
> @@ -466,8 +466,9 @@ int sja1105_static_config_upload(struct sja1105_private *priv)
>  				"invalid, retrying...\n");
>  			continue;
>  		}
> -	} while (--retries && (status.crcchkl == 1 || status.crcchkg == 1 ||
> -		 status.configs == 0 || status.ids == 1));
> +		/* Success! */
> +		break;
> +	} while (--retries);

Good point, I'll send a V2 for that. Thanks Dan for your keen eyes.

Colin

>  
>  	if (!retries) {
>  		rc = -EIO;
>