* [PATCH v2] dm raid: fix parse_raid_params() variable range issue
@ 2018-03-22 17:21 Heinz Mauelshagen
2018-03-22 19:41 ` Mike Snitzer
0 siblings, 1 reply; 5+ messages in thread
From: Heinz Mauelshagen @ 2018-03-22 17:21 UTC (permalink / raw)
To: heinzm, dm-devel, snitzer
This v2 addresses Mikulas' point about the variable range and folds in
"[PATCH] dm raid: use __within_range() more in parse_raid_params()":
parse_raid_parames() compared variable "int value" with
INT_MAX to prevent overflow of mddev variables set.
Change type to "long long value".
Whilst on it, use __within_range() throughout and
add a sync min/max rate check.
Signed-off-by: Heinz Mauelshagen <heinzm@redhat.com>
---
drivers/md/dm-raid.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index c1d1034ff7b7..c0e3d2aa9346 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -1141,7 +1141,7 @@ static int validate_raid_redundancy(struct raid_set *rs)
static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
unsigned int num_raid_params)
{
- int value, raid10_format = ALGORITHM_RAID10_DEFAULT;
+ long long value, raid10_format = ALGORITHM_RAID10_DEFAULT;
unsigned int raid10_copies = 2;
unsigned int i, write_mostly = 0;
unsigned int region_size = 0;
@@ -1153,7 +1153,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
arg = dm_shift_arg(as);
num_raid_params--; /* Account for chunk_size argument */
- if (kstrtoint(arg, 10, &value) < 0) {
+ if (kstrtoll(arg, 10, &value) < 0) {
rs->ti->error = "Bad numerical argument given for chunk_size";
return -EINVAL;
}
@@ -1315,7 +1315,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
/*
* Parameters with number values from here on.
*/
- if (kstrtoint(arg, 10, &value) < 0) {
+ if (kstrtoll(arg, 10, &value) < 0) {
rs->ti->error = "Bad numerical argument given in raid params";
return -EINVAL;
}
@@ -1430,7 +1430,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
rs->ti->error = "Only one min_recovery_rate argument pair allowed";
return -EINVAL;
}
- if (value > INT_MAX) {
+ if (!__within_range(value, 0, INT_MAX)) {
rs->ti->error = "min_recovery_rate out of range";
return -EINVAL;
}
@@ -1440,7 +1440,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
rs->ti->error = "Only one max_recovery_rate argument pair allowed";
return -EINVAL;
}
- if (value > INT_MAX) {
+ if (!__within_range(value, 0, INT_MAX)) {
rs->ti->error = "max_recovery_rate out of range";
return -EINVAL;
}
@@ -1472,6 +1472,12 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
}
}
+ if (rs->md.sync_speed_max &&
+ rs->md.sync_speed_max < rs->md.sync_speed_min) {
+ rs->ti->error = "sync speed max smaller than min";
+ return -EINVAL;
+ }
+
if (test_bit(__CTR_FLAG_SYNC, &rs->ctr_flags) &&
test_bit(__CTR_FLAG_NOSYNC, &rs->ctr_flags)) {
rs->ti->error = "sync and nosync are mutually exclusive";
--
2.14.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] dm raid: fix parse_raid_params() variable range issue
2018-03-22 17:21 [PATCH v2] dm raid: fix parse_raid_params() variable range issue Heinz Mauelshagen
@ 2018-03-22 19:41 ` Mike Snitzer
2018-03-22 21:13 ` Heinz Mauelshagen
0 siblings, 1 reply; 5+ messages in thread
From: Mike Snitzer @ 2018-03-22 19:41 UTC (permalink / raw)
To: Heinz Mauelshagen; +Cc: dm-devel
On Thu, Mar 22 2018 at 1:21pm -0400,
Heinz Mauelshagen <heinzm@redhat.com> wrote:
> This v2 addresses Mikulas' point about the variable range and folds in
> "[PATCH] dm raid: use __within_range() more in parse_raid_params()":
>
> parse_raid_parames() compared variable "int value" with
> INT_MAX to prevent overflow of mddev variables set.
>
> Change type to "long long value".
Can you elaborate on the risk/issue that is being fixed here?
User specifying a value that overflows an int?
(also: see below for inline comment about last hunk)
> Whilst on it, use __within_range() throughout and
> add a sync min/max rate check.
>
> Signed-off-by: Heinz Mauelshagen <heinzm@redhat.com>
> ---
> drivers/md/dm-raid.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index c1d1034ff7b7..c0e3d2aa9346 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -1141,7 +1141,7 @@ static int validate_raid_redundancy(struct raid_set *rs)
> static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> unsigned int num_raid_params)
> {
> - int value, raid10_format = ALGORITHM_RAID10_DEFAULT;
> + long long value, raid10_format = ALGORITHM_RAID10_DEFAULT;
> unsigned int raid10_copies = 2;
> unsigned int i, write_mostly = 0;
> unsigned int region_size = 0;
> @@ -1153,7 +1153,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> arg = dm_shift_arg(as);
> num_raid_params--; /* Account for chunk_size argument */
>
> - if (kstrtoint(arg, 10, &value) < 0) {
> + if (kstrtoll(arg, 10, &value) < 0) {
> rs->ti->error = "Bad numerical argument given for chunk_size";
> return -EINVAL;
> }
> @@ -1315,7 +1315,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> /*
> * Parameters with number values from here on.
> */
> - if (kstrtoint(arg, 10, &value) < 0) {
> + if (kstrtoll(arg, 10, &value) < 0) {
> rs->ti->error = "Bad numerical argument given in raid params";
> return -EINVAL;
> }
> @@ -1430,7 +1430,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> rs->ti->error = "Only one min_recovery_rate argument pair allowed";
> return -EINVAL;
> }
> - if (value > INT_MAX) {
> + if (!__within_range(value, 0, INT_MAX)) {
> rs->ti->error = "min_recovery_rate out of range";
> return -EINVAL;
> }
> @@ -1440,7 +1440,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> rs->ti->error = "Only one max_recovery_rate argument pair allowed";
> return -EINVAL;
> }
> - if (value > INT_MAX) {
> + if (!__within_range(value, 0, INT_MAX)) {
> rs->ti->error = "max_recovery_rate out of range";
> return -EINVAL;
> }
> @@ -1472,6 +1472,12 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> }
> }
>
> + if (rs->md.sync_speed_max &&
> + rs->md.sync_speed_max < rs->md.sync_speed_min) {
> + rs->ti->error = "sync speed max smaller than min";
> + return -EINVAL;
> + }
> +
> if (test_bit(__CTR_FLAG_SYNC, &rs->ctr_flags) &&
> test_bit(__CTR_FLAG_NOSYNC, &rs->ctr_flags)) {
> rs->ti->error = "sync and nosync are mutually exclusive";
> --
> 2.14.3
>
Isn't this last hunk unrelated?
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] dm raid: fix parse_raid_params() variable range issue
2018-03-22 19:41 ` Mike Snitzer
@ 2018-03-22 21:13 ` Heinz Mauelshagen
2018-03-26 18:16 ` Mike Snitzer
0 siblings, 1 reply; 5+ messages in thread
From: Heinz Mauelshagen @ 2018-03-22 21:13 UTC (permalink / raw)
To: Mike Snitzer; +Cc: dm-devel
On 03/22/2018 08:41 PM, Mike Snitzer wrote:
> On Thu, Mar 22 2018 at 1:21pm -0400,
> Heinz Mauelshagen <heinzm@redhat.com> wrote:
>
>> This v2 addresses Mikulas' point about the variable range and folds in
>> "[PATCH] dm raid: use __within_range() more in parse_raid_params()":
>>
>> parse_raid_parames() compared variable "int value" with
>> INT_MAX to prevent overflow of mddev variables set.
>>
>> Change type to "long long value".
> Can you elaborate on the risk/issue that is being fixed here?
Fix addresses a coverity finding supporting the full,
positive range of the "struct mddev" int members
set here. I.e. the "int" cast is compared with INT_MAX.
>
> User specifying a value that overflows an int?
No, kstroint() catches that.
>
> (also: see below for inline comment about last hunk)
See below...
>> Whilst on it, use __within_range() throughout and
>> add a sync min/max rate check.
>>
>> Signed-off-by: Heinz Mauelshagen <heinzm@redhat.com>
>> ---
>> drivers/md/dm-raid.c | 16 +++++++++++-----
>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
>> index c1d1034ff7b7..c0e3d2aa9346 100644
>> --- a/drivers/md/dm-raid.c
>> +++ b/drivers/md/dm-raid.c
>> @@ -1141,7 +1141,7 @@ static int validate_raid_redundancy(struct raid_set *rs)
>> static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
>> unsigned int num_raid_params)
>> {
>> - int value, raid10_format = ALGORITHM_RAID10_DEFAULT;
>> + long long value, raid10_format = ALGORITHM_RAID10_DEFAULT;
>> unsigned int raid10_copies = 2;
>> unsigned int i, write_mostly = 0;
>> unsigned int region_size = 0;
>> @@ -1153,7 +1153,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
>> arg = dm_shift_arg(as);
>> num_raid_params--; /* Account for chunk_size argument */
>>
>> - if (kstrtoint(arg, 10, &value) < 0) {
>> + if (kstrtoll(arg, 10, &value) < 0) {
>> rs->ti->error = "Bad numerical argument given for chunk_size";
>> return -EINVAL;
>> }
>> @@ -1315,7 +1315,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
>> /*
>> * Parameters with number values from here on.
>> */
>> - if (kstrtoint(arg, 10, &value) < 0) {
>> + if (kstrtoll(arg, 10, &value) < 0) {
>> rs->ti->error = "Bad numerical argument given in raid params";
>> return -EINVAL;
>> }
>> @@ -1430,7 +1430,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
>> rs->ti->error = "Only one min_recovery_rate argument pair allowed";
>> return -EINVAL;
>> }
>> - if (value > INT_MAX) {
>> + if (!__within_range(value, 0, INT_MAX)) {
>> rs->ti->error = "min_recovery_rate out of range";
>> return -EINVAL;
>> }
>> @@ -1440,7 +1440,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
>> rs->ti->error = "Only one max_recovery_rate argument pair allowed";
>> return -EINVAL;
>> }
>> - if (value > INT_MAX) {
>> + if (!__within_range(value, 0, INT_MAX)) {
>> rs->ti->error = "max_recovery_rate out of range";
>> return -EINVAL;
>> }
>> @@ -1472,6 +1472,12 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
>> }
>> }
>>
>> + if (rs->md.sync_speed_max &&
>> + rs->md.sync_speed_max < rs->md.sync_speed_min) {
>> + rs->ti->error = "sync speed max smaller than min";
>> + return -EINVAL;
>> + }
>> +
>> if (test_bit(__CTR_FLAG_SYNC, &rs->ctr_flags) &&
>> test_bit(__CTR_FLAG_NOSYNC, &rs->ctr_flags)) {
>> rs->ti->error = "sync and nosync are mutually exclusive";
>> --
>> 2.14.3
>>
> Isn't this last hunk unrelated?
No, once using __within_range to ensure positive values
for sync min/max, this hunk ensures that those are sane
if both are set.
Heinz
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] dm raid: fix parse_raid_params() variable range issue
2018-03-22 21:13 ` Heinz Mauelshagen
@ 2018-03-26 18:16 ` Mike Snitzer
2018-03-26 23:23 ` Mike Snitzer
0 siblings, 1 reply; 5+ messages in thread
From: Mike Snitzer @ 2018-03-26 18:16 UTC (permalink / raw)
To: Heinz Mauelshagen; +Cc: dm-devel
On Thu, Mar 22 2018 at 5:13pm -0400,
Heinz Mauelshagen <heinzm@redhat.com> wrote:
> On 03/22/2018 08:41 PM, Mike Snitzer wrote:
> >On Thu, Mar 22 2018 at 1:21pm -0400,
> >Heinz Mauelshagen <heinzm@redhat.com> wrote:
> >
> >>This v2 addresses Mikulas' point about the variable range and folds in
> >>"[PATCH] dm raid: use __within_range() more in parse_raid_params()":
> >>
> >>parse_raid_parames() compared variable "int value" with
> >>INT_MAX to prevent overflow of mddev variables set.
> >>
> >>Change type to "long long value".
> >Can you elaborate on the risk/issue that is being fixed here?
>
> Fix addresses a coverity finding supporting the full,
> positive range of the "struct mddev" int members
> set here. I.e. the "int" cast is compared with INT_MAX.
Can you cut and paste the relevant portions of the coverity report?
> >>Whilst on it, use __within_range() throughout and
> >>add a sync min/max rate check.
> >>
> >>Signed-off-by: Heinz Mauelshagen <heinzm@redhat.com>
> >>---
> >> drivers/md/dm-raid.c | 16 +++++++++++-----
> >> 1 file changed, 11 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> >>index c1d1034ff7b7..c0e3d2aa9346 100644
> >>--- a/drivers/md/dm-raid.c
> >>+++ b/drivers/md/dm-raid.c
> >>@@ -1141,7 +1141,7 @@ static int validate_raid_redundancy(struct raid_set *rs)
> >> static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> >> unsigned int num_raid_params)
> >> {
> >>- int value, raid10_format = ALGORITHM_RAID10_DEFAULT;
> >>+ long long value, raid10_format = ALGORITHM_RAID10_DEFAULT;
> >> unsigned int raid10_copies = 2;
> >> unsigned int i, write_mostly = 0;
> >> unsigned int region_size = 0;
> >>@@ -1153,7 +1153,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> >> arg = dm_shift_arg(as);
> >> num_raid_params--; /* Account for chunk_size argument */
> >>- if (kstrtoint(arg, 10, &value) < 0) {
> >>+ if (kstrtoll(arg, 10, &value) < 0) {
> >> rs->ti->error = "Bad numerical argument given for chunk_size";
> >> return -EINVAL;
> >> }
> >>@@ -1315,7 +1315,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> >> /*
> >> * Parameters with number values from here on.
> >> */
> >>- if (kstrtoint(arg, 10, &value) < 0) {
> >>+ if (kstrtoll(arg, 10, &value) < 0) {
> >> rs->ti->error = "Bad numerical argument given in raid params";
> >> return -EINVAL;
> >> }
> >>@@ -1430,7 +1430,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> >> rs->ti->error = "Only one min_recovery_rate argument pair allowed";
> >> return -EINVAL;
> >> }
> >>- if (value > INT_MAX) {
> >>+ if (!__within_range(value, 0, INT_MAX)) {
> >> rs->ti->error = "min_recovery_rate out of range";
> >> return -EINVAL;
> >> }
> >>@@ -1440,7 +1440,7 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> >> rs->ti->error = "Only one max_recovery_rate argument pair allowed";
> >> return -EINVAL;
> >> }
> >>- if (value > INT_MAX) {
> >>+ if (!__within_range(value, 0, INT_MAX)) {
> >> rs->ti->error = "max_recovery_rate out of range";
> >> return -EINVAL;
> >> }
> >>@@ -1472,6 +1472,12 @@ static int parse_raid_params(struct raid_set *rs, struct dm_arg_set *as,
> >> }
> >> }
> >>+ if (rs->md.sync_speed_max &&
> >>+ rs->md.sync_speed_max < rs->md.sync_speed_min) {
> >>+ rs->ti->error = "sync speed max smaller than min";
> >>+ return -EINVAL;
> >>+ }
> >>+
> >> if (test_bit(__CTR_FLAG_SYNC, &rs->ctr_flags) &&
> >> test_bit(__CTR_FLAG_NOSYNC, &rs->ctr_flags)) {
> >> rs->ti->error = "sync and nosync are mutually exclusive";
> >>--
> >>2.14.3
> >>
> >Isn't this last hunk unrelated?
>
> No, once using __within_range to ensure positive values
> for sync min/max, this hunk ensures that those are sane
> if both are set.
OK, but pretty much unrelated. I'll leave it foled in but at least
mention it in the header.
Mike
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] dm raid: fix parse_raid_params() variable range issue
2018-03-26 18:16 ` Mike Snitzer
@ 2018-03-26 23:23 ` Mike Snitzer
0 siblings, 0 replies; 5+ messages in thread
From: Mike Snitzer @ 2018-03-26 23:23 UTC (permalink / raw)
To: Heinz Mauelshagen; +Cc: dm-devel
On Mon, Mar 26 2018 at 2:16pm -0400,
Mike Snitzer <snitzer@redhat.com> wrote:
> On Thu, Mar 22 2018 at 5:13pm -0400,
> Heinz Mauelshagen <heinzm@redhat.com> wrote:
>
> > On 03/22/2018 08:41 PM, Mike Snitzer wrote:
> > >On Thu, Mar 22 2018 at 1:21pm -0400,
> > >Heinz Mauelshagen <heinzm@redhat.com> wrote:
> > >
> > >>This v2 addresses Mikulas' point about the variable range and folds in
> > >>"[PATCH] dm raid: use __within_range() more in parse_raid_params()":
> > >>
> > >>parse_raid_parames() compared variable "int value" with
> > >>INT_MAX to prevent overflow of mddev variables set.
> > >>
> > >>Change type to "long long value".
> > >Can you elaborate on the risk/issue that is being fixed here?
> >
> > Fix addresses a coverity finding supporting the full,
> > positive range of the "struct mddev" int members
> > set here. I.e. the "int" cast is compared with INT_MAX.
>
> Can you cut and paste the relevant portions of the coverity report?
I've dropped this patch for now. Until I get more insight on what the
problem is I'm not appreciating why changing to a larger data type is
the right way forward (especially if MD is just using int anyway).
Mike
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-26 23:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-22 17:21 [PATCH v2] dm raid: fix parse_raid_params() variable range issue Heinz Mauelshagen
2018-03-22 19:41 ` Mike Snitzer
2018-03-22 21:13 ` Heinz Mauelshagen
2018-03-26 18:16 ` Mike Snitzer
2018-03-26 23:23 ` Mike Snitzer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.