| 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
| ||
|
Message-ID: <e76fb64d-3a16-3cd5-c4a5-cb35bab15937@huawei.com>
Date: Thu, 19 Oct 2023 09:57:18 +0800
From: Zhihao Cheng <chengzhihao1@...wei.com>
To: ZhaoLong Wang <wangzhaolong1@...wei.com>, <richard@....at>,
<miquel.raynal@...tlin.com>, <vigneshr@...com>,
<dpervushin@...eddedalley.com>, <Artem.Bityutskiy@...ia.com>
CC: <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<yi.zhang@...wei.com>, <yangerkun@...wei.com>
Subject: Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by
ftl notifier
在 2023/10/18 20:16, ZhaoLong Wang 写道:
> If both flt.ko and gluebi.ko are loaded, the notiier of ftl
> triggers NULL pointer dereference when trying to access
> ‘gluebi->desc’ in gluebi_read().
>
> ubi_gluebi_init
> ubi_register_volume_notifier
> ubi_enumerate_volumes
> ubi_notify_all
> gluebi_notify nb->notifier_call()
> gluebi_create
> mtd_device_register
> mtd_device_parse_register
> add_mtd_device
> blktrans_notify_add not->add()
> ftl_add_mtd tr->add_mtd()
> scan_header
> mtd_read
> mtd_read
> mtd_read_oob
> gluebi_read mtd->read()
> gluebi->desc - NULL
>
> Detailed reproduction information available at the link[1],
>
> In the normal case, obtain gluebi->desc in the gluebi_get_device(),
> and accesses gluebi->desc in the gluebi_read(). However,
> gluebi_get_device() is not executed in advance in the
> ftl_add_mtd() process, which leads to NULL pointer dereference.
>
> The value of gluebi->desc may also be a negative error code, which
> triggers the page fault error.
>
> This patch has the following modifications:
>
> 1. Do not assign gluebi->desc to the error code. Use the NULL instead.
>
> 2. Always check the validity of gluebi->desc in gluebi_read() If the
> gluebi->desc is NULL, try to get MTD device.
>
> Such a modification currently works because the mutex "mtd_table_mutex"
> is held on all necessary paths, including the ftl_add_mtd() call path,
> open and close paths. Therefore, many race condition can be avoided.
>
> Fixes: 2ba3d76a1e29 ("UBI: make gluebi a separate module")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
> Signed-off-by: ZhaoLong Wang <wangzhaolong1@...wei.com>
> ---
> drivers/mtd/ubi/gluebi.c | 37 +++++++++++++++++++++++++++++++------
> 1 file changed, 31 insertions(+), 6 deletions(-)
>
Reviewed-by: Zhihao Cheng <chengzhihao1@...wei.com>
> diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
> index 1b980d15d9fb..0ca7f104adbf 100644
> --- a/drivers/mtd/ubi/gluebi.c
> +++ b/drivers/mtd/ubi/gluebi.c
> @@ -85,6 +85,7 @@ static int gluebi_get_device(struct mtd_info *mtd)
> {
> struct gluebi_device *gluebi;
> int ubi_mode = UBI_READONLY;
> + struct ubi_volume_desc *vdesc;
>
> if (mtd->flags & MTD_WRITEABLE)
> ubi_mode = UBI_READWRITE;
> @@ -109,12 +110,14 @@ static int gluebi_get_device(struct mtd_info *mtd)
> * This is the first reference to this UBI volume via the MTD device
> * interface. Open the corresponding volume in read-write mode.
> */
> - gluebi->desc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id,
> + vdesc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id,
> ubi_mode);
> - if (IS_ERR(gluebi->desc)) {
> + if (IS_ERR(vdesc)) {
> + gluebi->desc = NULL;
> mutex_unlock(&devices_mutex);
> - return PTR_ERR(gluebi->desc);
> + return PTR_ERR(vdesc);
> }
> + gluebi->desc = vdesc;
> gluebi->refcnt += 1;
> mutex_unlock(&devices_mutex);
> return 0;
> @@ -134,8 +137,10 @@ static void gluebi_put_device(struct mtd_info *mtd)
> gluebi = container_of(mtd, struct gluebi_device, mtd);
> mutex_lock(&devices_mutex);
> gluebi->refcnt -= 1;
> - if (gluebi->refcnt == 0)
> + if (gluebi->refcnt == 0) {
> ubi_close_volume(gluebi->desc);
> + gluebi->desc = NULL;
> + }
> mutex_unlock(&devices_mutex);
> }
>
> @@ -154,9 +159,26 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len,
> size_t *retlen, unsigned char *buf)
> {
> int err = 0, lnum, offs, bytes_left;
> - struct gluebi_device *gluebi;
> + struct gluebi_device *gluebi = container_of(mtd, struct gluebi_device,
> + mtd);
> + int no_desc = gluebi->desc == NULL ? 1 : 0;
> +
> + /**
> + * In normal case, the UBI volume desc has been initialized by
> + * ->_get_device(). However, in the ftl notifier process, the
> + * ->_get_device() is not executed in advance and the MTD device
> + * is directly scanned which cause NULL pointer dereference.
> + * Therefore, try to get the MTD device here.
> + */
> + if (unlikely(no_desc)) {
> + err = __get_mtd_device(mtd);
> + if (err) {
> + err_msg("cannot get MTD device %d, UBI device %d, volume %d, error %d",
> + mtd->index, gluebi->ubi_num, gluebi->vol_id, err);
> + return err;
> + }
> + }
>
> - gluebi = container_of(mtd, struct gluebi_device, mtd);
> lnum = div_u64_rem(from, mtd->erasesize, &offs);
> bytes_left = len;
> while (bytes_left) {
> @@ -176,6 +198,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len,
> }
>
> *retlen = len - bytes_left;
> +
> + if (unlikely(no_desc))
> + __put_mtd_device(mtd);
> return err;
> }
>
>
Powered by blists - more mailing lists