[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d089e4ff-26dc-22e4-58b3-8756f8ebaabc@huawei.com>
Date: Thu, 12 Oct 2023 16:18:07 +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>, <yangerkun@...wei.com>
Subject: Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by
ftl notifier
在 2023/10/12 16:04, ZhaoLong Wang 写道:
> I'm very happy to receive a reply to the review.
>
>> 2. fd = open(/dev/ubi0_0, O_WRONLY)
>> ubi_open_volume // vol->writers = 1
>>
>> P1 P2
>> gluebi_create -> mtd_device_register -> add_mtd_device:
>> device_register // dev/mtd1 is visible
>>
>> fd = open(/dev/mtd1, O_WRONLY)
>> gluebi_get_device
>> gluebi->desc = ubi_open_volume
>> gluebi->desc = ERR_PTR(EBUSY)
>>
>> ftl_add_mtd
>> mtd_read
>> gluebi_read
>> gluebi->desc is ERR_PTR (√)
>
> The reproduction steps for situations 2 and 3 have been added to link[1].
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
>
>> 3. P1 P2
>> gluebi_create -> mtd_device_register -> add_mtd_device:
>> device_register // dev/mtd1 is visible
>>
>> fd = open(/dev/mtd1, O_WRONLY)
>> gluebi_get_device
>> gluebi->desc = ubi_open_volume
>>
>> ftl_add_mtd
>> mtd_read
>> gluebi_read
>> gluebi->desc is not ERR_PTR/NULL
>>
>> close(fd)
>> gluebi_put_device
>> ubi_close_volume
>> kfree(desc)
>> ubi_read(gluebi->desc) // UAF (×)
>>
>
> Yes, it's also a problem. Perhaps it should be set to NULL after
> destroying gluebi->desc.
The key point is that 'gluebi->desc' check & usage is not atomic in
gluebi_read. So following patch still can't handle situation 3.
>
>>
>> No need to modify 'gluebi_write' and 'gluebi_erase'.
>>
>
> The patch is as follows:
>
> diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
> index 1b980d15d9fb..8fc6017d1155 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 isnt_get = unlikely(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 pointe dereference.
> + * Therefore, try to get the MTD device here.
> + */
> + if (unlikely(isnt_get)) {
> + 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(isnt_get))
> + __put_mtd_device(mtd);
> return err;
> }
>
>
>
> .
Powered by blists - more mailing lists