[PATCH 0/5] UBI: Coverity-inspired fixes

[PATCH 0/5] UBI: Coverity-inspired fixes

[Brian Norris]

Except for the last one, these were inspired by Coverity Scan results.

These fixes have barely been tested, but they are pretty straightforward
logically. As they've been sitting in my dust pile too long, I thought I'd at
least get them out there.

Brian Norris (5):
  UBI: account for bitflips in both the VID header and data
  UBI: fix out of bounds write
  UBI: initialize LEB number variable
  UBI: fix check for "too many bytes"
  UBI: align comment for readability

 drivers/mtd/ubi/attach.c | 2 +-
 drivers/mtd/ubi/cdev.c   | 2 +-
 drivers/mtd/ubi/eba.c    | 3 ++-
 drivers/mtd/ubi/ubi.h    | 2 +-
 drivers/mtd/ubi/wl.c     | 2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.1.0

[PATCH 1/5] UBI: account for bitflips in both the VID header and data

[Brian Norris]

We are completely discarding the earlier value of 'bitflips', which
could reflect a bitflip found in ubi_io_read_vid_hdr(). Let's use the
bitwise OR of header and data 'bitflip' statuses instead.

Coverity CID #1226856

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/ubi/attach.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 9d2e16f3150a..b5e154856994 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -410,7 +410,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct ubi_ainf_peb *aeb,
 		second_is_newer = !second_is_newer;
 	} else {
 		dbg_bld("PEB %d CRC is OK", pnum);
-		bitflips = !!err;
+		bitflips |= !!err;
 	}
 	mutex_unlock(&ubi->buf_mutex);
 
-- 
2.1.0

[PATCH 2/5] UBI: fix out of bounds write

[Brian Norris]

If aeb->len >= vol->reserved_pebs, we should not be writing aeb into the
PEB->LEB mapping.

Caught by Coverity, CID #711212.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/ubi/eba.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index da4c79259f67..6442d3fbcb73 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -1418,7 +1418,8 @@ int ubi_eba_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
 				 * during re-size.
 				 */
 				ubi_move_aeb_to_list(av, aeb, &ai->erase);
-			vol->eba_tbl[aeb->lnum] = aeb->pnum;
+			else
+				vol->eba_tbl[aeb->lnum] = aeb->pnum;
 		}
 	}
 
-- 
2.1.0

[PATCH 3/5] UBI: initialize LEB number variable

[Brian Norris]

In some of the 'out_not_moved' error paths, lnum may be used
uninitialized. Don't ignore the warning; let's fix it.

This uninitialized variable doesn't have much visible effect in the end,
since we just schedule the PEB for erasure, and its LEB number doesn't
really matter (it just gets printed in debug messages). But let's get it
straight anyway.

Coverity CID #113449

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/ubi/wl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 8f7bde6a85d6..0bd92d816391 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1002,7 +1002,7 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
 				int shutdown)
 {
 	int err, scrubbing = 0, torture = 0, protect = 0, erroneous = 0;
-	int vol_id = -1, uninitialized_var(lnum);
+	int vol_id = -1, lnum = -1;
 #ifdef CONFIG_MTD_UBI_FASTMAP
 	int anchor = wrk->anchor;
 #endif
-- 
2.1.0

[PATCH 4/5] UBI: fix check for "too many bytes"

[Brian Norris]

The comparison from the previous line seems to have been erroneously
(partially) copied-and-pasted onto the next. The second line should be
checking req.bytes, not req.lnum.

Coverity CID #139400

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/ubi/cdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index d647e504f9b1..7691a2a015cb 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -455,7 +455,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 		/* Validate the request */
 		err = -EINVAL;
 		if (req.lnum < 0 || req.lnum >= vol->reserved_pebs ||
-		    req.bytes < 0 || req.lnum >= vol->usable_leb_size)
+		    req.bytes < 0 || req.bytes >= vol->usable_leb_size)
 			break;
 
 		err = get_exclusive(desc);
-- 
2.1.0

Re: [PATCH 4/5] UBI: fix check for "too many bytes"

[Richard Weinberger]

Am 28.02.2015 um 11:23 schrieb Brian Norris:
> The comparison from the previous line seems to have been erroneously
> (partially) copied-and-pasted onto the next. The second line should be
> checking req.bytes, not req.lnum.
> 
> Coverity CID #139400
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/ubi/cdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
> index d647e504f9b1..7691a2a015cb 100644
> --- a/drivers/mtd/ubi/cdev.c
> +++ b/drivers/mtd/ubi/cdev.c
> @@ -455,7 +455,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
>  		/* Validate the request */
>  		err = -EINVAL;
>  		if (req.lnum < 0 || req.lnum >= vol->reserved_pebs ||
> -		    req.bytes < 0 || req.lnum >= vol->usable_leb_size)
> +		    req.bytes < 0 || req.bytes >= vol->usable_leb_size)
>  			break;

Testing uncovered an issue with that.
The comparison should be "req.bytes > vol->usable_leb_size" as
we're allowed to write with usable_leb_size.
There is no need to re-send, I'll fix the patch myself.

Thanks,
//richard

[PATCH 5/5] UBI: align comment for readability

[Brian Norris]

The kerneldoc for @vid_hdr_aloffset continues onto a second line, but
this is not obvious, because the second line isn't indented, and it
begins with '@'.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/ubi/ubi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index c5be82d9d345..2251a6c4c8fa 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -479,7 +479,7 @@ struct ubi_debug_info {
  * @vid_hdr_offset: starting offset of the volume identifier header (might be
  *                  unaligned)
  * @vid_hdr_aloffset: starting offset of the VID header aligned to
- * @hdrs_min_io_size
+ *                    @hdrs_min_io_size
  * @vid_hdr_shift: contains @vid_hdr_offset - @vid_hdr_aloffset
  * @bad_allowed: whether the MTD device admits of bad physical eraseblocks or
  *               not
-- 
2.1.0

Re: [PATCH 0/5] UBI: Coverity-inspired fixes

[Richard Weinberger]

Brian,

Am 28.02.2015 um 11:23 schrieb Brian Norris:
> Except for the last one, these were inspired by Coverity Scan results.
> 
> These fixes have barely been tested, but they are pretty straightforward
> logically. As they've been sitting in my dust pile too long, I thought I'd at
> least get them out there.
> 
> Brian Norris (5):
>   UBI: account for bitflips in both the VID header and data
>   UBI: fix out of bounds write
>   UBI: initialize LEB number variable
>   UBI: fix check for "too many bytes"
>   UBI: align comment for readability

Nice work!
I'll test them later today.
Just a quick question, no patch has a stable tag, is this by design?
>From a first look most of them look like stable material.

Thanks,
//richard

Re: [PATCH 0/5] UBI: Coverity-inspired fixes

[Brian Norris]

On Thu, Mar 05, 2015 at 11:33:14AM +0100, Richard Weinberger wrote:
> Brian,
> 
> Am 28.02.2015 um 11:23 schrieb Brian Norris:
> > Except for the last one, these were inspired by Coverity Scan results.
> > 
> > These fixes have barely been tested, but they are pretty straightforward
> > logically. As they've been sitting in my dust pile too long, I thought I'd at
> > least get them out there.
> > 
> > Brian Norris (5):
> >   UBI: account for bitflips in both the VID header and data
> >   UBI: fix out of bounds write
> >   UBI: initialize LEB number variable
> >   UBI: fix check for "too many bytes"
> >   UBI: align comment for readability
> 
> Nice work!
> I'll test them later today.
> Just a quick question, no patch has a stable tag, is this by design?
> From a first look most of them look like stable material.

Two reasons:

 1. I hadn't tested them heavily, and I definitely didn't try to target
 their codepaths much.

 2. Given #1 and the fact that these were just found by static analysis,
 I don't think they pass this test from
 Documentation/stable_kernel_rules.txt:

 " - It must fix a real bug that bothers people (not a, "This could be a
    problem..." type thing)."

So, I expected they would only be sent to stable if somebody (perhaps
me) is able to trigger something real, or at least gets some significant
testing on them.

Maybe this is a case where you send the fixes, and then send the commit
IDs to Greg after they have been proven stable and/or can be exploited
in some way through testing. (Option 2 in the updated
stable_kernel_rules.txt.)

But really, it's your/Artem's call.

Brian

Re: [PATCH 0/5] UBI: Coverity-inspired fixes

[Richard Weinberger]

Am 06.03.2015 um 03:04 schrieb Brian Norris:
> On Thu, Mar 05, 2015 at 11:33:14AM +0100, Richard Weinberger wrote:
>> Brian,
>>
>> Am 28.02.2015 um 11:23 schrieb Brian Norris:
>>> Except for the last one, these were inspired by Coverity Scan results.
>>>
>>> These fixes have barely been tested, but they are pretty straightforward
>>> logically. As they've been sitting in my dust pile too long, I thought I'd at
>>> least get them out there.
>>>
>>> Brian Norris (5):
>>>   UBI: account for bitflips in both the VID header and data
>>>   UBI: fix out of bounds write
>>>   UBI: initialize LEB number variable
>>>   UBI: fix check for "too many bytes"
>>>   UBI: align comment for readability
>>
>> Nice work!
>> I'll test them later today.
>> Just a quick question, no patch has a stable tag, is this by design?
>> From a first look most of them look like stable material.
> 
> Two reasons:
> 
>  1. I hadn't tested them heavily, and I definitely didn't try to target
>  their codepaths much.
> 
>  2. Given #1 and the fact that these were just found by static analysis,
>  I don't think they pass this test from
>  Documentation/stable_kernel_rules.txt:
> 
>  " - It must fix a real bug that bothers people (not a, "This could be a
>     problem..." type thing)."
> 
> So, I expected they would only be sent to stable if somebody (perhaps
> me) is able to trigger something real, or at least gets some significant
> testing on them.
> 
> Maybe this is a case where you send the fixes, and then send the commit
> IDs to Greg after they have been proven stable and/or can be exploited
> in some way through testing. (Option 2 in the updated
> stable_kernel_rules.txt.)
> 
> But really, it's your/Artem's call.

Applied, thanks a lot Brian!
I've marked patches 1 to 4 as stable material.

Thanks,
//richard