| Message ID | tencent_0D24426FAC6A21B69AC0C03CE4143A508F09@qq.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [v2] md: raid1: fix potential OOB in raid1_remove_disk() | expand |
在 2023/07/22 15:53, Zhang Shurong 写道: > If rddev->raid_disk is greater than mddev->raid_disks, there will be > an out-of-bounds in raid1_remove_disk(). We have already found > similar reports as follows: > > 1) commit d17f744e883b ("md-raid10: fix KASAN warning") > 2) commit 1ebc2cec0b7d ("dm raid: fix KASAN warning in raid5_remove_disk") > > Fix this bug by checking whether the "number" variable is > valid. LGTM Reviewed-by: Yu Kuai <yukuai3@huawei.com> > > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> > --- > Changes in v2: > - Using conf->raid_disks instead of mddev->raid_disks. > > drivers/md/raid1.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index dd25832eb045..80aeee63dfb7 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -1829,6 +1829,10 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev) > struct r1conf *conf = mddev->private; > int err = 0; > int number = rdev->raid_disk; > + > + if (unlikely(number >= conf->raid_disks)) > + goto abort; > + > struct raid1_info *p = conf->mirrors + number; > > if (rdev != p->rdev) >
On Mon, Jul 24, 2023 at 10:12 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > 在 2023/07/22 15:53, Zhang Shurong 写道: > > If rddev->raid_disk is greater than mddev->raid_disks, there will be > > an out-of-bounds in raid1_remove_disk(). We have already found > > similar reports as follows: > > > > 1) commit d17f744e883b ("md-raid10: fix KASAN warning") > > 2) commit 1ebc2cec0b7d ("dm raid: fix KASAN warning in raid5_remove_disk") > > > > Fix this bug by checking whether the "number" variable is > > valid. > > LGTM > > Reviewed-by: Yu Kuai <yukuai3@huawei.com> > > > > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> > > --- > > Changes in v2: > > - Using conf->raid_disks instead of mddev->raid_disks. > > > > drivers/md/raid1.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > > index dd25832eb045..80aeee63dfb7 100644 > > --- a/drivers/md/raid1.c > > +++ b/drivers/md/raid1.c > > @@ -1829,6 +1829,10 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev) > > struct r1conf *conf = mddev->private; > > int err = 0; > > int number = rdev->raid_disk; > > + > > + if (unlikely(number >= conf->raid_disks)) > > + goto abort; We need err = -EINVAL here. > > + > > struct raid1_info *p = conf->mirrors + number; > > > > if (rdev != p->rdev) > > >
Hi, 在 2023/07/29 18:49, Song Liu 写道: > On Mon, Jul 24, 2023 at 10:12 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: >> >> 在 2023/07/22 15:53, Zhang Shurong 写道: >>> If rddev->raid_disk is greater than mddev->raid_disks, there will be >>> an out-of-bounds in raid1_remove_disk(). We have already found >>> similar reports as follows: >>> >>> 1) commit d17f744e883b ("md-raid10: fix KASAN warning") >>> 2) commit 1ebc2cec0b7d ("dm raid: fix KASAN warning in raid5_remove_disk") >>> >>> Fix this bug by checking whether the "number" variable is >>> valid. >> >> LGTM >> >> Reviewed-by: Yu Kuai <yukuai3@huawei.com> >>> >>> Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> >>> --- >>> Changes in v2: >>> - Using conf->raid_disks instead of mddev->raid_disks. >>> >>> drivers/md/raid1.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c >>> index dd25832eb045..80aeee63dfb7 100644 >>> --- a/drivers/md/raid1.c >>> +++ b/drivers/md/raid1.c >>> @@ -1829,6 +1829,10 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev) >>> struct r1conf *conf = mddev->private; >>> int err = 0; >>> int number = rdev->raid_disk; >>> + >>> + if (unlikely(number >= conf->raid_disks)) >>> + goto abort; > > We need err = -EINVAL here. I think return 0 is right here, so that caller can remove this rdev from array successfully, this only need to return error for the case -EBUSY. Thanks, Kuai > >>> + >>> struct raid1_info *p = conf->mirrors + number; >>> >>> if (rdev != p->rdev) >>> >> > . >
On Mon, Jul 31, 2023 at 5:08 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > Hi, > > 在 2023/07/29 18:49, Song Liu 写道: > > On Mon, Jul 24, 2023 at 10:12 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > >> > >> 在 2023/07/22 15:53, Zhang Shurong 写道: > >>> If rddev->raid_disk is greater than mddev->raid_disks, there will be > >>> an out-of-bounds in raid1_remove_disk(). We have already found > >>> similar reports as follows: > >>> > >>> 1) commit d17f744e883b ("md-raid10: fix KASAN warning") > >>> 2) commit 1ebc2cec0b7d ("dm raid: fix KASAN warning in raid5_remove_disk") > >>> > >>> Fix this bug by checking whether the "number" variable is > >>> valid. > >> > >> LGTM > >> > >> Reviewed-by: Yu Kuai <yukuai3@huawei.com> > >>> > >>> Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> > >>> --- > >>> Changes in v2: > >>> - Using conf->raid_disks instead of mddev->raid_disks. > >>> > >>> drivers/md/raid1.c | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > >>> index dd25832eb045..80aeee63dfb7 100644 > >>> --- a/drivers/md/raid1.c > >>> +++ b/drivers/md/raid1.c > >>> @@ -1829,6 +1829,10 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev) > >>> struct r1conf *conf = mddev->private; > >>> int err = 0; > >>> int number = rdev->raid_disk; > >>> + > >>> + if (unlikely(number >= conf->raid_disks)) > >>> + goto abort; > > > > We need err = -EINVAL here. > > I think return 0 is right here, so that caller can remove this rdev > from array successfully, this only need to return error for the case > -EBUSY. Ah, that's right. Applied to md-next. Thanks, Song
On Fri, Sep 8, 2023 at 6:48 AM Nigel Croxon <ncroxon@redhat.com> wrote: > > > On 7/22/23 3:53 AM, Zhang Shurong wrote: > > If rddev->raid_disk is greater than mddev->raid_disks, there will be > an out-of-bounds in raid1_remove_disk(). We have already found > similar reports as follows: > > 1) commit d17f744e883b ("md-raid10: fix KASAN warning") > 2) commit 1ebc2cec0b7d ("dm raid: fix KASAN warning in raid5_remove_disk") > > Fix this bug by checking whether the "number" variable is > valid. > > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> > --- > Changes in v2: > - Using conf->raid_disks instead of mddev->raid_disks. > > drivers/md/raid1.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index dd25832eb045..80aeee63dfb7 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -1829,6 +1829,10 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev) > struct r1conf *conf = mddev->private; > int err = 0; > int number = rdev->raid_disk; > + > + if (unlikely(number >= conf->raid_disks)) > + goto abort; > + > struct raid1_info *p = conf->mirrors + number; > > if (rdev != p->rdev) > > When compiling this patch.. I get the following error > > drivers/md/raid1.c: In function 'raid1_remove_disk': > drivers/md/raid1.c:1844:9: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] > 1844 | struct raid1_info *p = conf->mirrors + number; > | ^~~~~~ > > And that's because the new code was inserted before the struct. > Here is a fix: > > raid1: fix error: ISO C90 forbids mixed declarations > > There is a compile error when commit is added: > md: raid1: fix potential OOB in raid1_remove_disk() > > drivers/md/raid1.c: In function 'raid1_remove_disk': > drivers/md/raid1.c:1844:9: error: ISO C90 forbids mixed declarations > and code [-Werror=declaration-after-statement] > 1844 | struct raid1_info *p = conf->mirrors + number; > | ^~~~~~ > > And that's because the new code was inserted before the struct. > The change is move the struct command above the new commit. > > Fixes: md: raid1: fix potential OOB in raid1_remove_disk() > commit 8b0472b50bcf > > Signed-off-by: Nigel Croxon <ncroxon@redhat.com> Thanks for catching this! Would you mind sending an official patch for it? Song > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index a5453b126aab..4f1483bb708b 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -1846,11 +1846,11 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev) > int err = 0; > int number = rdev->raid_disk; > > + struct raid1_info *p = conf->mirrors + number; > + > if (unlikely(number >= conf->raid_disks)) > goto abort; > > - struct raid1_info *p = conf->mirrors + number; > - > if (rdev != p->rdev) > p = conf->mirrors + conf->raid_disks + number; > >
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index dd25832eb045..80aeee63dfb7 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -1829,6 +1829,10 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev) struct r1conf *conf = mddev->private; int err = 0; int number = rdev->raid_disk; + + if (unlikely(number >= conf->raid_disks)) + goto abort; + struct raid1_info *p = conf->mirrors + number; if (rdev != p->rdev)
If rddev->raid_disk is greater than mddev->raid_disks, there will be an out-of-bounds in raid1_remove_disk(). We have already found similar reports as follows: 1) commit d17f744e883b ("md-raid10: fix KASAN warning") 2) commit 1ebc2cec0b7d ("dm raid: fix KASAN warning in raid5_remove_disk") Fix this bug by checking whether the "number" variable is valid. Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> --- Changes in v2: - Using conf->raid_disks instead of mddev->raid_disks. drivers/md/raid1.c | 4 ++++ 1 file changed, 4 insertions(+)