re: dm: raid456 basic support

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]

On Fri, Jun 17 2016, Dan Carpenter wrote:

> [ No idea why it's only just now complaining about issues from 2011... ]
>
> Hello NeilBrown,

Hello ... sorry for the delay in replying.


>
> The patch 9d09e663d550: "dm: raid456 basic support" from Jan 13,
> 2011, leads to the following static checker warning:
>
> 	drivers/md/dm-raid.c:1217 parse_raid_params()
> 	warn: no lower bound on 'value'
>
> drivers/md/dm-raid.c
>   1211                                  return -EINVAL;
>   1212                          }
>   1213                          if (!value || (value > MAX_SCHEDULE_TIMEOUT)) {
>
> value is an int.  MAX_SCHEDULE_TIMEOUT is LONG_MAX.  Should it be
> INT_MAX?  What about negatives?

% $ git show 9d09e663d550 | grep 'value;' | head -n1
+	unsigned long value;


I think value is unsigned long.
It is set on two occasions with:
  strict_strtoul(argv[0], 10, &value)

and we bail out if that fails.

The first time we assign it to an int ({new_,}chunk_sectors) without
range checking, which is bad.

We cast it to an int for calling raid5_set_cache_size() without first
range checking, which is bad.

Might either of these be the cause of the rather peculiar warning?

The following patch (against mainline) should fix those issues.
Do they silence your warning?

Thanks,
NeilBrown

diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 52532745a50f..670d237a26a9 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -520,6 +520,9 @@ static int parse_raid_params(struct raid_set *rs, char **argv,
 	} else if (value < 8) {
 		rs->ti->error = "Chunk size value is too small";
 		return -EINVAL;
+	} else if (value > INT_MAX) {
+		rs->ti->error = "Chunk size value is too large";
+		return -EINVAL;
 	}
 
 	rs->md.new_chunk_sectors = rs->md.chunk_sectors = value;
@@ -650,7 +653,8 @@ static int parse_raid_params(struct raid_set *rs, char **argv,
 				rs->ti->error = "Inappropriate argument: stripe_cache";
 				return -EINVAL;
 			}
-			if (raid5_set_cache_size(&rs->md, (int)value)) {
+			if (value > INT_MAX ||
+			    raid5_set_cache_size(&rs->md, (int)value)) {
 				rs->ti->error = "Bad stripe_cache size";
 				return -EINVAL;
 			}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]