| Message ID | 20210527145048.795954-1-colin.king@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [next] mtd: rawnand: ensure return variable is initialized | expand |
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
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 >
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
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
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
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
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;