[ 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

On Fri, Mar 3, 2023 at 8:31 AM Dan Carpenter <error27@...> 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.

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

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@...> 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