linux-kernel - Re: [PATCH 1/2] ubi: fix slab-out-of-bounds in ubi_eba_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cdb047f0-c071-3263-0167-e9bce852b5d5@huawei.com>
Date:   Thu, 6 Apr 2023 20:14:52 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     ZhaoLong Wang <wangzhaolong1@...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

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