* [bug report] md: range check slot number when manually adding a spare.
@ 2023-03-03 14:13 Dan Carpenter
2023-03-03 16:09 ` Roger Heflin
2023-03-05 22:36 ` NeilBrown
0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2023-03-03 14:13 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, cip-dev
[ Ancient code, but you're still at the same email address... -dan ]
Hello NeilBrown,
The patch ba1b41b6b4e3: "md: range check slot number when manually
adding a spare." from Jan 14, 2011, leads to the following Smatch
static checker warning:
drivers/md/md.c:3170 slot_store() warn: no lower bound on 'slot'
drivers/md/md.c:3172 slot_store() warn: no lower bound on 'slot'
drivers/md/md.c:3190 slot_store() warn: no lower bound on 'slot'
drivers/md/md.c
3117 static ssize_t
3118 slot_store(struct md_rdev *rdev, const char *buf, size_t len)
3119 {
3120 int slot;
3121 int err;
3122
3123 if (test_bit(Journal, &rdev->flags))
3124 return -EBUSY;
3125 if (strncmp(buf, "none", 4)==0)
3126 slot = -1;
3127 else {
3128 err = kstrtouint(buf, 10, (unsigned int *)&slot);
slot comes from the user.
3129 if (err < 0)
3130 return err;
3131 }
3132 if (rdev->mddev->pers && slot == -1) {
3133 /* Setting 'slot' on an active array requires also
3134 * updating the 'rd%d' link, and communicating
3135 * with the personality with ->hot_*_disk.
3136 * For now we only support removing
3137 * failed/spare devices. This normally happens automatically,
3138 * but not when the metadata is externally managed.
3139 */
3140 if (rdev->raid_disk == -1)
3141 return -EEXIST;
3142 /* personality does all needed checks */
3143 if (rdev->mddev->pers->hot_remove_disk == NULL)
3144 return -EINVAL;
3145 clear_bit(Blocked, &rdev->flags);
3146 remove_and_add_spares(rdev->mddev, rdev);
3147 if (rdev->raid_disk >= 0)
3148 return -EBUSY;
3149 set_bit(MD_RECOVERY_NEEDED, &rdev->mddev->recovery);
3150 md_wakeup_thread(rdev->mddev->thread);
3151 } else if (rdev->mddev->pers) {
3152 /* Activating a spare .. or possibly reactivating
3153 * if we ever get bitmaps working here.
3154 */
3155 int err;
3156
3157 if (rdev->raid_disk != -1)
3158 return -EBUSY;
3159
3160 if (test_bit(MD_RECOVERY_RUNNING, &rdev->mddev->recovery))
3161 return -EBUSY;
3162
3163 if (rdev->mddev->pers->hot_add_disk == NULL)
3164 return -EINVAL;
3165
3166 if (slot >= rdev->mddev->raid_disks &&
3167 slot >= rdev->mddev->raid_disks + rdev->mddev->delta_disks)
3168 return -ENOSPC;
-1 is valid, but should this check if slot < -1?
3169
--> 3170 rdev->raid_disk = slot;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] md: range check slot number when manually adding a spare.
2023-03-03 14:13 [bug report] md: range check slot number when manually adding a spare Dan Carpenter
@ 2023-03-03 16:09 ` Roger Heflin
2023-03-03 17:15 ` Dan Carpenter
2023-03-05 22:36 ` NeilBrown
1 sibling, 1 reply; 4+ messages in thread
From: Roger Heflin @ 2023-03-03 16:09 UTC (permalink / raw)
To: Dan Carpenter; +Cc: neilb, linux-raid, cip-dev
On Fri, Mar 3, 2023 at 8:31 AM Dan Carpenter <error27@gmail.com> wrote:
>
> [ Ancient code, but you're still at the same email address... -dan ]
>
> Hello NeilBrown,
>
> The patch ba1b41b6b4e3: "md: range check slot number when manually
> adding a spare." from Jan 14, 2011, leads to the following Smatch
> static checker warning:
>
> drivers/md/md.c:3170 slot_store() warn: no lower bound on 'slot'
> drivers/md/md.c:3172 slot_store() warn: no lower bound on 'slot'
> drivers/md/md.c:3190 slot_store() warn: no lower bound on 'slot'
>
> drivers/md/md.c
> 3117 static ssize_t
> 3118 slot_store(struct md_rdev *rdev, const char *buf, size_t len)
> 3119 {
> 3120 int slot;
> 3121 int err;
> 3122
> 3123 if (test_bit(Journal, &rdev->flags))
> 3124 return -EBUSY;
> 3125 if (strncmp(buf, "none", 4)==0)
> 3126 slot = -1;
> 3127 else {
> 3128 err = kstrtouint(buf, 10, (unsigned int *)&slot);
>
> slot comes from the user.
>
> 3129 if (err < 0)
> 3130 return err;
> 3131 }
kstrtouint is string to unsigned int, it has this code:
if (tmp != (unsigned int)tmp)
return -ERANGE;
And so will not return a negative number without an error, so 0 is the
lower limit as enforced by the function.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] md: range check slot number when manually adding a spare.
2023-03-03 16:09 ` Roger Heflin
@ 2023-03-03 17:15 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2023-03-03 17:15 UTC (permalink / raw)
To: Roger Heflin; +Cc: neilb, linux-raid, cip-dev
On Fri, Mar 03, 2023 at 10:09:50AM -0600, Roger Heflin wrote:
> On Fri, Mar 3, 2023 at 8:31 AM Dan Carpenter <error27@gmail.com> wrote:
> >
> > [ Ancient code, but you're still at the same email address... -dan ]
> >
> > Hello NeilBrown,
> >
> > The patch ba1b41b6b4e3: "md: range check slot number when manually
> > adding a spare." from Jan 14, 2011, leads to the following Smatch
> > static checker warning:
> >
> > drivers/md/md.c:3170 slot_store() warn: no lower bound on 'slot'
> > drivers/md/md.c:3172 slot_store() warn: no lower bound on 'slot'
> > drivers/md/md.c:3190 slot_store() warn: no lower bound on 'slot'
> >
> > drivers/md/md.c
> > 3117 static ssize_t
> > 3118 slot_store(struct md_rdev *rdev, const char *buf, size_t len)
> > 3119 {
> > 3120 int slot;
> > 3121 int err;
> > 3122
> > 3123 if (test_bit(Journal, &rdev->flags))
> > 3124 return -EBUSY;
> > 3125 if (strncmp(buf, "none", 4)==0)
> > 3126 slot = -1;
> > 3127 else {
> > 3128 err = kstrtouint(buf, 10, (unsigned int *)&slot);
> >
> > slot comes from the user.
> >
> > 3129 if (err < 0)
> > 3130 return err;
> > 3131 }
>
> kstrtouint is string to unsigned int, it has this code:
>
> if (tmp != (unsigned int)tmp)
> return -ERANGE;
>
> And so will not return a negative number without an error, so 0 is the
> lower limit as enforced by the function.
Some of what you have written is correct, but your main conclusion is
wrong. The kstrtouint() gives you unsigned ints. If you take a very
large unsigned int and cast it to an int then you get a negative value
so the underflow issue is real.
Btw, in more modern code we would write:
err = kstrtoint(buf, 10, &slot);
if (err)
return err;
It still has the underflow issue... I have been wanting to say that and
resisted saying it because it was obvious to everyone. However I am
only human and can only resist the temptation to point out the obvious
for so long.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] md: range check slot number when manually adding a spare.
2023-03-03 14:13 [bug report] md: range check slot number when manually adding a spare Dan Carpenter
2023-03-03 16:09 ` Roger Heflin
@ 2023-03-05 22:36 ` NeilBrown
1 sibling, 0 replies; 4+ messages in thread
From: NeilBrown @ 2023-03-05 22:36 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-raid, cip-dev
On Sat, 04 Mar 2023, Dan Carpenter wrote:
> [ Ancient code, but you're still at the same email address... -dan ]
Patch sent. Thanks for the report.
NeilBrown
>
> Hello NeilBrown,
>
> The patch ba1b41b6b4e3: "md: range check slot number when manually
> adding a spare." from Jan 14, 2011, leads to the following Smatch
> static checker warning:
>
> drivers/md/md.c:3170 slot_store() warn: no lower bound on 'slot'
> drivers/md/md.c:3172 slot_store() warn: no lower bound on 'slot'
> drivers/md/md.c:3190 slot_store() warn: no lower bound on 'slot'
>
> drivers/md/md.c
> 3117 static ssize_t
> 3118 slot_store(struct md_rdev *rdev, const char *buf, size_t len)
> 3119 {
> 3120 int slot;
> 3121 int err;
> 3122
> 3123 if (test_bit(Journal, &rdev->flags))
> 3124 return -EBUSY;
> 3125 if (strncmp(buf, "none", 4)==0)
> 3126 slot = -1;
> 3127 else {
> 3128 err = kstrtouint(buf, 10, (unsigned int *)&slot);
>
> slot comes from the user.
>
> 3129 if (err < 0)
> 3130 return err;
> 3131 }
> 3132 if (rdev->mddev->pers && slot == -1) {
> 3133 /* Setting 'slot' on an active array requires also
> 3134 * updating the 'rd%d' link, and communicating
> 3135 * with the personality with ->hot_*_disk.
> 3136 * For now we only support removing
> 3137 * failed/spare devices. This normally happens automatically,
> 3138 * but not when the metadata is externally managed.
> 3139 */
> 3140 if (rdev->raid_disk == -1)
> 3141 return -EEXIST;
> 3142 /* personality does all needed checks */
> 3143 if (rdev->mddev->pers->hot_remove_disk == NULL)
> 3144 return -EINVAL;
> 3145 clear_bit(Blocked, &rdev->flags);
> 3146 remove_and_add_spares(rdev->mddev, rdev);
> 3147 if (rdev->raid_disk >= 0)
> 3148 return -EBUSY;
> 3149 set_bit(MD_RECOVERY_NEEDED, &rdev->mddev->recovery);
> 3150 md_wakeup_thread(rdev->mddev->thread);
> 3151 } else if (rdev->mddev->pers) {
> 3152 /* Activating a spare .. or possibly reactivating
> 3153 * if we ever get bitmaps working here.
> 3154 */
> 3155 int err;
> 3156
> 3157 if (rdev->raid_disk != -1)
> 3158 return -EBUSY;
> 3159
> 3160 if (test_bit(MD_RECOVERY_RUNNING, &rdev->mddev->recovery))
> 3161 return -EBUSY;
> 3162
> 3163 if (rdev->mddev->pers->hot_add_disk == NULL)
> 3164 return -EINVAL;
> 3165
> 3166 if (slot >= rdev->mddev->raid_disks &&
> 3167 slot >= rdev->mddev->raid_disks + rdev->mddev->delta_disks)
> 3168 return -ENOSPC;
>
> -1 is valid, but should this check if slot < -1?
>
> 3169
> --> 3170 rdev->raid_disk = slot;
>
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-05 22:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-03 14:13 [bug report] md: range check slot number when manually adding a spare Dan Carpenter
2023-03-03 16:09 ` Roger Heflin
2023-03-03 17:15 ` Dan Carpenter
2023-03-05 22:36 ` NeilBrown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).