* re: dm: raid456 basic support
@ 2016-06-17 9:14 Dan Carpenter
2016-07-07 23:57 ` NeilBrown
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2016-06-17 9:14 UTC (permalink / raw)
To: neilb; +Cc: linux-raid
[ No idea why it's only just now complaining about issues from 2011... ]
Hello NeilBrown,
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?
1214 rs->ti->error = "daemon sleep period out of range";
1215 return -EINVAL;
1216 }
1217 rs->md.bitmap_info.daemon_sleep = value;
1218 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_DATA_OFFSET))) {
1219 /* Userspace passes new data_offset after having extended the the data image LV */
1220 if (test_and_set_bit(__CTR_FLAG_DATA_OFFSET, &rs->ctr_flags)) {
1221 rs->ti->error = "Only one data_offset argument pair allowed";
1222 return -EINVAL;
1223 }
1224 /* Ensure sensible data offset */
1225 if (value < 0) {
1226 rs->ti->error = "Bogus data_offset value";
1227 return -EINVAL;
1228 }
1229 rs->data_offset = value;
1230 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_DELTA_DISKS))) {
1231 /* Define the +/-# of disks to add to/remove from the given raid set */
1232 if (test_and_set_bit(__CTR_FLAG_DELTA_DISKS, &rs->ctr_flags)) {
1233 rs->ti->error = "Only one delta_disks argument pair allowed";
1234 return -EINVAL;
1235 }
1236 /* Ensure MAX_RAID_DEVICES and raid type minimal_devs! */
1237 if (!__within_range(abs(value), 1, MAX_RAID_DEVICES - rt->minimal_devs)) {
1238 rs->ti->error = "Too many delta_disk requested";
1239 return -EINVAL;
1240 }
1241
1242 rs->delta_disks = value;
1243 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_STRIPE_CACHE))) {
1244 if (test_and_set_bit(__CTR_FLAG_STRIPE_CACHE, &rs->ctr_flags)) {
1245 rs->ti->error = "Only one stripe_cache argument pair allowed";
1246 return -EINVAL;
1247 }
1248
1249 if (!rt_is_raid456(rt)) {
1250 rs->ti->error = "Inappropriate argument: stripe_cache";
1251 return -EINVAL;
1252 }
1253
1254 rs->stripe_cache_entries = value;
1255 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_MIN_RECOVERY_RATE))) {
1256 if (test_and_set_bit(__CTR_FLAG_MIN_RECOVERY_RATE, &rs->ctr_flags)) {
1257 rs->ti->error = "Only one min_recovery_rate argument pair allowed";
1258 return -EINVAL;
1259 }
1260 if (value > INT_MAX) {
^^^^^^^
Here we're using INT_MAX.
1261 rs->ti->error = "min_recovery_rate out of range";
1262 return -EINVAL;
1263 }
1264 rs->md.sync_speed_min = (int)value;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This looks like negatives are intentional... A few lines later as well.
drivers/md/dm-raid.c:1274 parse_raid_params() warn: no lower bound on 'value'
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* re: dm: raid456 basic support
2016-06-17 9:14 dm: raid456 basic support Dan Carpenter
@ 2016-07-07 23:57 ` NeilBrown
2016-07-08 14:13 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: NeilBrown @ 2016-07-07 23:57 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-raid
[-- 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 --]
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: dm: raid456 basic support
2016-07-07 23:57 ` NeilBrown
@ 2016-07-08 14:13 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2016-07-08 14:13 UTC (permalink / raw)
To: NeilBrown, Heinz Mauelshagen; +Cc: linux-raid, Mike Snitzer
Oops. Sorry. No, that won't help. We're looking at different code,
hence the confusion. I'm working off linux-next.
The problem is commit 6e20902e8f9e ('dm raid: fix failed
takeover/reshapes by keeping raid set frozen') where we changed "value"
to unsigned to int, but forgot to add underflow checks.
The warnings are:
drivers/md/dm-raid.c:1217 parse_raid_params() warn: no lower bound on 'value'
drivers/md/dm-raid.c:1264 parse_raid_params() warn: no lower bound on 'value'
drivers/md/dm-raid.c:1274 parse_raid_params() warn: no lower bound on 'value'
drivers/md/dm-raid.c
1147 if (kstrtoint(arg, 10, &value) < 0) {
^^^^^^
Set here.
1148 rs->ti->error = "Bad numerical argument given in raid params";
1149 return -EINVAL;
1150 }
1151
1152 if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_REBUILD))) {
1153 /*
1154 * "rebuild" is being passed in by userspace to provide
1155 * indexes of replaced devices and to set up additional
1156 * devices on raid level takeover.
1157 */
1158 if (!__within_range(value, 0, rs->raid_disks - 1)) {
1159 rs->ti->error = "Invalid rebuild index given";
1160 return -EINVAL;
1161 }
1162
1163 if (test_and_set_bit(value, (void *) rs->rebuild_disks)) {
1164 rs->ti->error = "rebuild for this index already given";
1165 return -EINVAL;
1166 }
1167
1168 rd = rs->dev + value;
1169 clear_bit(In_sync, &rd->rdev.flags);
1170 clear_bit(Faulty, &rd->rdev.flags);
1171 rd->rdev.recovery_offset = 0;
1172 set_bit(__CTR_FLAG_REBUILD, &rs->ctr_flags);
1173 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_WRITE_MOSTLY))) {
1174 if (!rt_is_raid1(rt)) {
1175 rs->ti->error = "write_mostly option is only valid for RAID1";
1176 return -EINVAL;
1177 }
1178
1179 if (!__within_range(value, 0, rs->md.raid_disks - 1)) {
1180 rs->ti->error = "Invalid write_mostly index given";
1181 return -EINVAL;
1182 }
1183
1184 set_bit(WriteMostly, &rs->dev[value].rdev.flags);
1185 set_bit(__CTR_FLAG_WRITE_MOSTLY, &rs->ctr_flags);
1186 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_MAX_WRITE_BEHIND))) {
1187 if (!rt_is_raid1(rt)) {
1188 rs->ti->error = "max_write_behind option is only valid for RAID1";
1189 return -EINVAL;
1190 }
1191
1192 if (test_and_set_bit(__CTR_FLAG_MAX_WRITE_BEHIND, &rs->ctr_flags)) {
1193 rs->ti->error = "Only one max_write_behind argument pair allowed";
1194 return -EINVAL;
1195 }
1196
1197 /*
1198 * In device-mapper, we specify things in sectors, but
1199 * MD records this value in kB
1200 */
1201 value /= 2;
1202 if (value > COUNTER_MAX) {
1203 rs->ti->error = "Max write-behind limit out of range";
1204 return -EINVAL;
1205 }
1206
1207 rs->md.bitmap_info.max_write_behind = value;
1208 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_DAEMON_SLEEP))) {
1209 if (test_and_set_bit(__CTR_FLAG_DAEMON_SLEEP, &rs->ctr_flags)) {
1210 rs->ti->error = "Only one daemon_sleep argument pair allowed";
1211 return -EINVAL;
1212 }
1213 if (!value || (value > MAX_SCHEDULE_TIMEOUT)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Not checked for negatives.
1214 rs->ti->error = "daemon sleep period out of range";
1215 return -EINVAL;
1216 }
1217 rs->md.bitmap_info.daemon_sleep = value;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-07-08 14:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-17 9:14 dm: raid456 basic support Dan Carpenter
2016-07-07 23:57 ` NeilBrown
2016-07-08 14:13 ` Dan Carpenter
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.