[PATCH][next] mtd: rawnand: ensure return variable is initialized

[PATCH][next] mtd: rawnand: ensure return variable is initialized

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Currently there are corner cases where spec_times is NULL and
chip->parameters.onfi or when best_mode is zero where ret is
not assigned a value and an uninitialized return value can be
returned. Fix this by ensuring ret is initialized to -EINVAL.

Addresses-Coverity: ("Uninitialized scalar variable")
Fixes: 9d3194bf2aef ("mtd: rawnand: Allow SDR timings to be nacked")
Fixes: a9ecc8c814e9 ("mtd: rawnand: Choose the best timings, NV-DDR included")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/mtd/nand/raw/nand_base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 57a583149cc0..18db742f650c 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -926,7 +926,7 @@ int nand_choose_best_sdr_timings(struct nand_chip *chip,
 				 struct nand_sdr_timings *spec_timings)
 {
 	const struct nand_controller_ops *ops = chip->controller->ops;
-	int best_mode = 0, mode, ret;
+	int best_mode = 0, mode, ret = -EINVAL;
 
 	iface->type = NAND_SDR_IFACE;
 
@@ -977,7 +977,7 @@ int nand_choose_best_nvddr_timings(struct nand_chip *chip,
 				   struct nand_nvddr_timings *spec_timings)
 {
 	const struct nand_controller_ops *ops = chip->controller->ops;
-	int best_mode = 0, mode, ret;
+	int best_mode = 0, mode, ret = 0;
 
 	iface->type = NAND_NVDDR_IFACE;
 
-- 
2.31.1

Re: [PATCH][next] mtd: rawnand: ensure return variable is initialized

[Miquel Raynal]

Hi Colin,

Colin King <colin.king@canonical.com> wrote on Thu, 27 May 2021
15:50:48 +0100:

> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently there are corner cases where spec_times is NULL and
> chip->parameters.onfi or when best_mode is zero where ret is

                       ^
something is missing here, the sentence is not clear

> not assigned a value and an uninitialized return value can be
> returned. Fix this by ensuring ret is initialized to -EINVAL.

I don't see how this situation can happen.

In both cases, no matter the value of best_mode, the for loop will
always execute at least one time (mode 0) so ret will be populated.

Maybe the robot does not know that best_mode cannot be negative and
should be defined unsigned, but the current patch is invalid.

> Addresses-Coverity: ("Uninitialized scalar variable")
> Fixes: 9d3194bf2aef ("mtd: rawnand: Allow SDR timings to be nacked")
> Fixes: a9ecc8c814e9 ("mtd: rawnand: Choose the best timings, NV-DDR included")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 57a583149cc0..18db742f650c 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -926,7 +926,7 @@ int nand_choose_best_sdr_timings(struct nand_chip *chip,
>  				 struct nand_sdr_timings *spec_timings)
>  {
>  	const struct nand_controller_ops *ops = chip->controller->ops;
> -	int best_mode = 0, mode, ret;
> +	int best_mode = 0, mode, ret = -EINVAL;
>  
>  	iface->type = NAND_SDR_IFACE;
>  
> @@ -977,7 +977,7 @@ int nand_choose_best_nvddr_timings(struct nand_chip *chip,
>  				   struct nand_nvddr_timings *spec_timings)
>  {
>  	const struct nand_controller_ops *ops = chip->controller->ops;
> -	int best_mode = 0, mode, ret;
> +	int best_mode = 0, mode, ret = 0;
>  
>  	iface->type = NAND_NVDDR_IFACE;
>  

Thanks,
Miquèl

Re: [PATCH][next] mtd: rawnand: ensure return variable is initialized

[Colin Ian King]

On 27/05/2021 16:03, Miquel Raynal wrote:
> Hi Colin,
> 
> Colin King <colin.king@canonical.com> wrote on Thu, 27 May 2021
> 15:50:48 +0100:
> 
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently there are corner cases where spec_times is NULL and
>> chip->parameters.onfi or when best_mode is zero where ret is
> 
>                        ^
> something is missing here, the sentence is not clear
> 
>> not assigned a value and an uninitialized return value can be
>> returned. Fix this by ensuring ret is initialized to -EINVAL.
> 
> I don't see how this situation can happen.
> 
> In both cases, no matter the value of best_mode, the for loop will
> always execute at least one time (mode 0) so ret will be populated.
> 
> Maybe the robot does not know that best_mode cannot be negative and
> should be defined unsigned, but the current patch is invalid.

Yep, I've looked at this again and it does seem like a false positive.
Apologies for the noise.

> 
>> Addresses-Coverity: ("Uninitialized scalar variable")
>> Fixes: 9d3194bf2aef ("mtd: rawnand: Allow SDR timings to be nacked")
>> Fixes: a9ecc8c814e9 ("mtd: rawnand: Choose the best timings, NV-DDR included")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/mtd/nand/raw/nand_base.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
>> index 57a583149cc0..18db742f650c 100644
>> --- a/drivers/mtd/nand/raw/nand_base.c
>> +++ b/drivers/mtd/nand/raw/nand_base.c
>> @@ -926,7 +926,7 @@ int nand_choose_best_sdr_timings(struct nand_chip *chip,
>>  				 struct nand_sdr_timings *spec_timings)
>>  {
>>  	const struct nand_controller_ops *ops = chip->controller->ops;
>> -	int best_mode = 0, mode, ret;
>> +	int best_mode = 0, mode, ret = -EINVAL;
>>  
>>  	iface->type = NAND_SDR_IFACE;
>>  
>> @@ -977,7 +977,7 @@ int nand_choose_best_nvddr_timings(struct nand_chip *chip,
>>  				   struct nand_nvddr_timings *spec_timings)
>>  {
>>  	const struct nand_controller_ops *ops = chip->controller->ops;
>> -	int best_mode = 0, mode, ret;
>> +	int best_mode = 0, mode, ret = 0;
>>  
>>  	iface->type = NAND_NVDDR_IFACE;
>>  
> 
> Thanks,
> Miquèl
> 

Re: [PATCH][next] mtd: rawnand: ensure return variable is initialized

[Dan Carpenter]

On Thu, May 27, 2021 at 05:03:09PM +0200, Miquel Raynal wrote:
> Hi Colin,
> 
> Colin King <colin.king@canonical.com> wrote on Thu, 27 May 2021
> 15:50:48 +0100:
> 
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Currently there are corner cases where spec_times is NULL and
> > chip->parameters.onfi or when best_mode is zero where ret is
> 
>                        ^
> something is missing here, the sentence is not clear
> 
> > not assigned a value and an uninitialized return value can be
> > returned. Fix this by ensuring ret is initialized to -EINVAL.
> 
> I don't see how this situation can happen.
> 
> In both cases, no matter the value of best_mode, the for loop will
> always execute at least one time (mode 0) so ret will be populated.
> 
> Maybe the robot does not know that best_mode cannot be negative and
> should be defined unsigned, but the current patch is invalid.
>

People think list counter unsigned is a good idea, but it's a terrible
idea and has caused hundreds of bugs for me to fix/report over the
years.  *grumble*.

Anyway, I was revisiting this code because it showed up as a Smatch
warning and the bug appears to be real.

	best_mode = fls(chip->parameters.onfi->sdr_timing_modes) - 1;

The "onfi->sdr_timing_modes" comes from the hardware in nand_onfi_detect()
and nothing checks that it is non-zero so "best_mode = fls(0) - 1;" is
negative and "ret" is uninitialized.

regards,
dan carpenter

Re: [PATCH][next] mtd: rawnand: ensure return variable is initialized

[Dan Carpenter]

On Tue, Jun 01, 2021 at 03:14:02PM +0300, Dan Carpenter wrote:
> On Thu, May 27, 2021 at 05:03:09PM +0200, Miquel Raynal wrote:
> > Hi Colin,
> > 
> > Colin King <colin.king@canonical.com> wrote on Thu, 27 May 2021
> > 15:50:48 +0100:
> > 
> > > From: Colin Ian King <colin.king@canonical.com>
> > > 
> > > Currently there are corner cases where spec_times is NULL and
> > > chip->parameters.onfi or when best_mode is zero where ret is
> > 
> >                        ^
> > something is missing here, the sentence is not clear
> > 
> > > not assigned a value and an uninitialized return value can be
> > > returned. Fix this by ensuring ret is initialized to -EINVAL.
> > 
> > I don't see how this situation can happen.
> > 
> > In both cases, no matter the value of best_mode, the for loop will
> > always execute at least one time (mode 0) so ret will be populated.
> > 
> > Maybe the robot does not know that best_mode cannot be negative and
> > should be defined unsigned, but the current patch is invalid.
> >
> 
> People think list counter unsigned is a good idea, but it's a terrible
> idea and has caused hundreds of bugs for me to fix/report over the
> years.  *grumble*.

Imagine if best_mode were unsigned int (and the loop ended on > 0 so it
wasn't an endless loop).  Then instead of a no-op the loop would iterate
4 million times.  Each iteration would trigger the WARN_ON()
onfi_fill_sdr_interface_config().

I think people believe that the compiler will warn them something like:
"warning: Assigning a subtract operation to an unsigned!" but the
compiler is never going to do that.  Unsigned is just a declaration that
"I'm never going to be surprised so let's make this stuff more dangerous
and fun!"

There are times where it's appropriate, sure.  But it's mostly unsigned
long which is correct instead of unsigned int.  If you were to draw a
number line from 0-U64_MAX then the band between 2-4 million is quite
skinny and a long way from the zero.  There aren't that many thing which
fall into the band.  Most numbers are smaller, but once we start talking
about millions then 4 million is very limitting so we would want to use
a 64 bit type.

regards,
dan carpenter

Re: [PATCH][next] mtd: rawnand: ensure return variable is initialized

[Miquel Raynal]

Hi Dan,

Dan Carpenter <dan.carpenter@oracle.com> wrote on Tue, 1 Jun 2021
15:14:02 +0300:

> On Thu, May 27, 2021 at 05:03:09PM +0200, Miquel Raynal wrote:
> > Hi Colin,
> > 
> > Colin King <colin.king@canonical.com> wrote on Thu, 27 May 2021
> > 15:50:48 +0100:
> >   
> > > From: Colin Ian King <colin.king@canonical.com>
> > > 
> > > Currently there are corner cases where spec_times is NULL and
> > > chip->parameters.onfi or when best_mode is zero where ret is  
> > 
> >                        ^
> > something is missing here, the sentence is not clear
> >   
> > > not assigned a value and an uninitialized return value can be
> > > returned. Fix this by ensuring ret is initialized to -EINVAL.  
> > 
> > I don't see how this situation can happen.
> > 
> > In both cases, no matter the value of best_mode, the for loop will
> > always execute at least one time (mode 0) so ret will be populated.
> > 
> > Maybe the robot does not know that best_mode cannot be negative and
> > should be defined unsigned, but the current patch is invalid.
> >  
> 
> People think list counter unsigned is a good idea, but it's a terrible
> idea and has caused hundreds of bugs for me to fix/report over the
> years.  *grumble*.
> 
> Anyway, I was revisiting this code because it showed up as a Smatch
> warning and the bug appears to be real.
> 
> 	best_mode = fls(chip->parameters.onfi->sdr_timing_modes) - 1;
> 
> The "onfi->sdr_timing_modes" comes from the hardware in nand_onfi_detect()
> and nothing checks that it is non-zero so "best_mode = fls(0) - 1;" is
> negative and "ret" is uninitialized.

In the ONFI specification, the sdr_timing_mode field is defined as
follow:

SDR timing mode support
BIT  VALUE MEANING
6-15 N/A   Reserved (0)
5    1     supports timing mode 5
4    1     supports timing mode 4
3    1     supports timing mode 3
2    1     supports timing mode 2
1    1     supports timing mode 1
0    1     supports timing mode 0, shall be 1

IOW sdr_timing_modes *cannot* be 0, or it is a truly deep and crazily
impacting hardware bug (so far I am not aware of any chip not returning
the right timing mode 0 value). Hence my proposal to turn best_mode as
unsigned. I honestly don't know what is the best option here and am
fully open to other suggestions to silence the robot.

Thanks,
Miquèl

Re: [PATCH][next] mtd: rawnand: ensure return variable is initialized

[Dan Carpenter]

On Mon, Jun 07, 2021 at 08:57:11AM +0200, Miquel Raynal wrote:
> 
> In the ONFI specification, the sdr_timing_mode field is defined as
> follow:
> 
> SDR timing mode support
> BIT  VALUE MEANING
> 6-15 N/A   Reserved (0)
> 5    1     supports timing mode 5
> 4    1     supports timing mode 4
> 3    1     supports timing mode 3
> 2    1     supports timing mode 2
> 1    1     supports timing mode 1
> 0    1     supports timing mode 0, shall be 1
> 
> IOW sdr_timing_modes *cannot* be 0, or it is a truly deep and crazily
> impacting hardware bug (so far I am not aware of any chip not returning
> the right timing mode 0 value). Hence my proposal to turn best_mode as
> unsigned. I honestly don't know what is the best option here and am
> fully open to other suggestions to silence the robot.

If the hardware is broken we should just return -EINVAL.

regards,
dan carpenter