> On Oct 28, 2019, at 8:41 PM, Yufen Yu <yuyufen@xxxxxxxxxx> wrote:
>
> we need to gurantee 'desc_nr' valid before access array of sb->dev_roles.
>
> Reported-by: coverity-bot <keescook+coverity-bot@xxxxxxxxxxxx>
> Addresses-Coverity-ID: 1487373 ("Memory - illegal accesses")
> Fixes: 6a5cb53aaa4e ("md: no longer compare spare disk superblock events in super_load")
> Signed-off-by: Yufen Yu <yuyufen@xxxxxxxxxx>
> ---
> drivers/md/md.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index fc6ae8276a92..8832ab70e34d 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1153,7 +1153,8 @@ static int super_90_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor
> * Insist on good event counter while assembling, except
> * for spares (which don't need an event count)
> */
> - if (sb->disks[rdev->desc_nr].state & (
> + if (rdev->desc_nr >= 0 &&
> + sb->disks[rdev->desc_nr].state & (
> (1<<MD_DISK_SYNC) | (1 << MD_DISK_ACTIVE)))
> ret = 1;
> else
So, we get ret = 0 for LEVEL_MULTIPATH. Then refdev would always
be NULL. This doesn't look right.
> @@ -1178,7 +1179,8 @@ static int super_90_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor
> * Insist on good event counter while assembling, except
> * for spares (which don't need an event count)
> */
> - if (sb->disks[rdev->desc_nr].state & (
> + if (rdev->desc_nr >= 0 &&
> + sb->disks[rdev->desc_nr].state & (
> (1<<MD_DISK_SYNC) | (1 << MD_DISK_ACTIVE)) &&
> (ev1 > ev2))
> ret = 1;
> @@ -1540,7 +1542,6 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
> sector_t sectors;
> char b[BDEVNAME_SIZE], b2[BDEVNAME_SIZE];
> int bmask;
> - __u64 role;
Hmm... why did we use __u64 in the first place...
>
> /*
> * Calculate the position of the superblock in 512byte sectors.
> @@ -1674,8 +1675,6 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
> sb->level != 0)
> return -EINVAL;
>
> - role = le16_to_cpu(sb->dev_roles[rdev->desc_nr]);
> -
> if (!refdev) {
> /*
> * Insist of good event counter while assembling, except for
> @@ -1683,8 +1682,8 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
> */
> if (rdev->desc_nr >= 0 &&
> rdev->desc_nr < le32_to_cpu(sb->max_dev) &&
> - (role < MD_DISK_ROLE_MAX ||
> - role == MD_DISK_ROLE_JOURNAL))
> + (le16_to_cpu(sb->dev_roles[rdev->desc_nr]) < MD_DISK_ROLE_MAX ||
> + le16_to_cpu(sb->dev_roles[rdev->desc_nr]) == MD_DISK_ROLE_JOURNAL))
Same concern for LEVEL_MULTIPATH applies here. And maybe we can make this
code simpler?
Thanks,
Song
> ret = 1;
> else
> ret = 0;
> @@ -1710,8 +1709,9 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
> */
> if (rdev->desc_nr >= 0 &&
> rdev->desc_nr < le32_to_cpu(sb->max_dev) &&
> - (role < MD_DISK_ROLE_MAX ||
> - role == MD_DISK_ROLE_JOURNAL) && ev1 > ev2)
> + (le16_to_cpu(sb->dev_roles[rdev->desc_nr]) < MD_DISK_ROLE_MAX ||
> + le16_to_cpu(sb->dev_roles[rdev->desc_nr]) == MD_DISK_ROLE_JOURNAL)
> + && ev1 > ev2)
> ret = 1;
> else
> ret = 0;
> --
> 2.17.2
>
