[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <307f61a1-b72c-c310-797c-013a8914ec1c@huawei.com>
Date: Thu, 4 May 2023 10:12:29 +0800
From: ZhaoLong Wang <wangzhaolong1@...wei.com>
To: Zhihao Cheng <chengzhihao1@...wei.com>, <richard@....at>,
<miquel.raynal@...tlin.com>, <vigneshr@...com>
CC: <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<yi.zhang@...wei.com>
Subject: Re: [PATCH 1/2] ubi: fix slab-out-of-bounds in
ubi_eba_get_ldesc+0xfb/0x130
Yes, that could happen. I was able to reproduce the problem despite the
low probability of triggering it.
This race between wear_leveling_work() and ubi_resize_volume() can cause
data corruption in the UBIFS running on the UBI volume.. ubi->volumes_lock
must be added to protect the update of eba_tbl in the ubi_eba_copy_leb().
I'll do a V2 patch later to fix this issue.
With appreciation
ZhaoLong Wang
> HI,
>> From: Guo Xuenan <guoxuenan@...wei.com>
>>
>> When using ioctl interface to resize ubi volume, ubi_resize_volume will
>> resize eba table first, but not change vol->reserved_pebs in the same
>> atomic context which may cause concurrency access eba table.
>>
>> For example, When user do shrink ubi volume A calling ubi_resize_volume,
>> while the other thread is writing (volume B) and triggering
>> wear-leveling,
>> which may calling ubi_write_fastmap, under these circumstances, KASAN
>> may
>> report: slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130.
>>
> [...]
>> diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
>> index 2c867d16f89f..97294def01eb 100644
>> --- a/drivers/mtd/ubi/vmt.c
>> +++ b/drivers/mtd/ubi/vmt.c
>> @@ -408,6 +408,7 @@ int ubi_resize_volume(struct ubi_volume_desc
>> *desc, int reserved_pebs)
>> struct ubi_device *ubi = vol->ubi;
>> struct ubi_vtbl_record vtbl_rec;
>> struct ubi_eba_table *new_eba_tbl = NULL;
>> + struct ubi_eba_table *old_eba_tbl = NULL;
>> int vol_id = vol->vol_id;
>> if (ubi->ro_mode)
>> @@ -453,10 +454,13 @@ int ubi_resize_volume(struct ubi_volume_desc
>> *desc, int reserved_pebs)
>> err = -ENOSPC;
>> goto out_free;
>> }
>> +
>> ubi->avail_pebs -= pebs;
>> ubi->rsvd_pebs += pebs;
>> ubi_eba_copy_table(vol, new_eba_tbl, vol->reserved_pebs);
>> - ubi_eba_replace_table(vol, new_eba_tbl);
>> + old_eba_tbl = vol->eba_tbl;
>> + vol->eba_tbl = new_eba_tbl;
>> + vol->reserved_pebs = reserved_pebs;
>> spin_unlock(&ubi->volumes_lock);
>> }
>> @@ -471,7 +475,9 @@ int ubi_resize_volume(struct ubi_volume_desc
>> *desc, int reserved_pebs)
>> ubi->avail_pebs -= pebs;
>> ubi_update_reserved(ubi);
>> ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
>> - ubi_eba_replace_table(vol, new_eba_tbl);
>> + old_eba_tbl = vol->eba_tbl;
>> + vol->eba_tbl = new_eba_tbl;
>> + vol->reserved_pebs = reserved_pebs;
>> spin_unlock(&ubi->volumes_lock);
>> }
>> @@ -493,7 +499,6 @@ int ubi_resize_volume(struct ubi_volume_desc
>> *desc, int reserved_pebs)
>> if (err)
>> goto out_acc;
>> - vol->reserved_pebs = reserved_pebs;
>> if (vol->vol_type == UBI_DYNAMIC_VOLUME) {
>> vol->used_ebs = reserved_pebs;
>> vol->last_eb_bytes = vol->usable_leb_size;
>> @@ -501,19 +506,24 @@ int ubi_resize_volume(struct ubi_volume_desc
>> *desc, int reserved_pebs)
>> (long long)vol->used_ebs * vol->usable_leb_size;
>> }
>> + /* destroy old table */
>> + ubi_eba_destroy_table(old_eba_tbl);
>> ubi_volume_notify(ubi, vol, UBI_VOLUME_RESIZED);
>> self_check_volumes(ubi);
>> return err;
>> out_acc:
>> + spin_lock(&ubi->volumes_lock);
>> + vol->reserved_pebs = reserved_pebs - pebs;
>> if (pebs > 0) {
>> - spin_lock(&ubi->volumes_lock);
>> ubi->rsvd_pebs -= pebs;
>> ubi->avail_pebs += pebs;
>> - spin_unlock(&ubi->volumes_lock);
>> + ubi_eba_copy_table(vol, old_eba_tbl, vol->reserved_pebs);
>> + } else {
>> + ubi_eba_copy_table(vol, old_eba_tbl, reserved_pebs);
>> }
>> - return err;
>> -
>> + vol->eba_tbl = old_eba_tbl;
>> + spin_unlock(&ubi->volumes_lock);
>> out_free:
>> ubi_eba_destroy_table(new_eba_tbl);
>> return err;
>>
>
>
> Besides that, it's better to protect 'vol->eba_tbl->entries'
> assignment like:
> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> index 403b79d6efd5..5ae0c1bc6f41 100644
> --- a/drivers/mtd/ubi/eba.c
> +++ b/drivers/mtd/ubi/eba.c
> @@ -1450,7 +1450,9 @@ int ubi_eba_copy_leb(struct ubi_device *ubi, int
> from, int to,
> }
>
> ubi_assert(vol->eba_tbl->entries[lnum].pnum == from);
> + spin_lock(&ubi->volumes_lock);
> vol->eba_tbl->entries[lnum].pnum = to;
> + spin_unlock(&ubi->volumes_lock);
>
> out_unlock_buf:
> mutex_unlock(&ubi->buf_mutex);
>
> Otherwise, a race between wear_leveling_work and shrinking volume
> could happen:
>
> ubi_resize_volume wear_leveling_worker
> ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
> vol->eba_tbl->entries[lnum].pnum = to; // update old eba_tbl
> vol->eba_tbl = new_eba_tbl
Powered by blists - more mailing lists