[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f1b27408-6eae-4195-4c97-c286c4ad04db@huawei.com>
Date: Thu, 12 Oct 2023 19:25:39 +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 17:31, ZhaoLong Wang 写道:
>
>>>
>>>> 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.
>>
>
> Setting the desc to NULL works because
> mutex_lock "mtd_table_mutex" is held on all paths where
> ftl_add_mtd() is called.
>
Oh, you're right. Just one nit below:
> @@ -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;
This 'unlikey' can be removed.
Rename 'isnt_get' as 'has_desc' ?
> + /**
> + * 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;
> + }
> + }
Powered by blists - more mailing lists