[PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[ZhaoLong Wang]

If both flt.ko and gluebi.ko are loaded, the notiier of ftl
triggers NULL pointer dereference when trying to visit
‘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.

This patch assumes that the gluebi module is not designed to work with
the ftl module. In this case, the patch only needs to prevent the ftl
notifier operation.

Add some correctness check for gluebi->desc in gluebi_read/write/erase(),
If the pointer is invalid, the -EINVAL is returned.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
---
 drivers/mtd/ubi/gluebi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
index 1b980d15d9fb..189ecc0eacd1 100644
--- a/drivers/mtd/ubi/gluebi.c
+++ b/drivers/mtd/ubi/gluebi.c
@@ -157,6 +157,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len,
 	struct gluebi_device *gluebi;
 
 	gluebi = container_of(mtd, struct gluebi_device, mtd);
+	if (IS_ERR_OR_NULL(gluebi->desc))
+		return -EINVAL;
+
 	lnum = div_u64_rem(from, mtd->erasesize, &offs);
 	bytes_left = len;
 	while (bytes_left) {
@@ -197,6 +200,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len,
 	struct gluebi_device *gluebi;
 
 	gluebi = container_of(mtd, struct gluebi_device, mtd);
+	if (IS_ERR_OR_NULL(gluebi->desc))
+		return -EINVAL;
+
 	lnum = div_u64_rem(to, mtd->erasesize, &offs);
 
 	if (len % mtd->writesize || offs % mtd->writesize)
@@ -242,6 +248,8 @@ static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr)
 	lnum = mtd_div_by_eb(instr->addr, mtd);
 	count = mtd_div_by_eb(instr->len, mtd);
 	gluebi = container_of(mtd, struct gluebi_device, mtd);
+	if (IS_ERR_OR_NULL(gluebi->desc))
+		return -EINVAL;
 
 	for (i = 0; i < count - 1; i++) {
 		err = ubi_leb_unmap(gluebi->desc, lnum + i);
-- 
2.31.1

Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[ZhaoLong Wang]

> This patch assumes that the gluebi module is not designed to work with
> the ftl module. In this case, the patch only needs to prevent the ftl
> notifier operation.
> 
> Add some correctness check for gluebi->desc in gluebi_read/write/erase(),
> If the pointer is invalid, the -EINVAL is returned.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
> Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
> ---
>   drivers/mtd/ubi/gluebi.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
> index 1b980d15d9fb..189ecc0eacd1 100644
> --- a/drivers/mtd/ubi/gluebi.c
> +++ b/drivers/mtd/ubi/gluebi.c
> @@ -157,6 +157,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len,
>   	struct gluebi_device *gluebi;
>   
>   	gluebi = container_of(mtd, struct gluebi_device, mtd);
> +	if (IS_ERR_OR_NULL(gluebi->desc))
> +		return -EINVAL;
> +
>   	lnum = div_u64_rem(from, mtd->erasesize, &offs);
>   	bytes_left = len;
>   	while (bytes_left) {
> @@ -197,6 +200,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len,
>   	struct gluebi_device *gluebi;
>   
>   	gluebi = container_of(mtd, struct gluebi_device, mtd);
> +	if (IS_ERR_OR_NULL(gluebi->desc))
> +		return -EINVAL;
> +
>   	lnum = div_u64_rem(to, mtd->erasesize, &offs);
>   
>   	if (len % mtd->writesize || offs % mtd->writesize)
> @@ -242,6 +248,8 @@ static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr)
>   	lnum = mtd_div_by_eb(instr->addr, mtd);
>   	count = mtd_div_by_eb(instr->len, mtd);
>   	gluebi = container_of(mtd, struct gluebi_device, mtd);
> +	if (IS_ERR_OR_NULL(gluebi->desc))
> +		return -EINVAL;
>   
>   	for (i = 0; i < count - 1; i++) {
>   		err = ubi_leb_unmap(gluebi->desc, lnum + i);


This modification attempts another solution. Always check the validity
of gluebi->desc. If the gluebi->desc pointer is invalid, try to get MTD
device.


diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
index 1b980d15d9fb..f1a74ccf1718 100644
--- a/drivers/mtd/ubi/gluebi.c
+++ b/drivers/mtd/ubi/gluebi.c
@@ -154,9 +154,19 @@ 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 not_get = IS_ERR_OR_NULL(gluebi->desc);
+
+	if (not_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 +186,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t 
from, size_t len,
  	}

  	*retlen = len - bytes_left;
+
+	if (not_get)
+		__put_mtd_device(mtd);
  	return err;
  }

@@ -194,9 +207,19 @@ static int gluebi_write(struct mtd_info *mtd, 
loff_t to, size_t len,
  			size_t *retlen, const u_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 not_get = IS_ERR_OR_NULL(gluebi->desc);
+
+	if (not_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(to, mtd->erasesize, &offs);

  	if (len % mtd->writesize || offs % mtd->writesize)
@@ -220,6 +243,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t 
to, size_t len,
  	}

  	*retlen = len - bytes_left;
+
+	if (not_get)
+		__put_mtd_device(mtd);
  	return err;
  }

@@ -234,14 +260,24 @@ static int gluebi_write(struct mtd_info *mtd, 
loff_t to, size_t len,
  static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr)
  {
  	int err, i, lnum, count;
-	struct gluebi_device *gluebi;
+	struct gluebi_device *gluebi = container_of(mtd, struct gluebi_device,
+						    mtd);
+	int not_get = IS_ERR_OR_NULL(gluebi->desc);
+
+	if (not_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;
+		}
+	}

  	if (mtd_mod_by_ws(instr->addr, mtd) || mtd_mod_by_ws(instr->len, mtd))
  		return -EINVAL;

  	lnum = mtd_div_by_eb(instr->addr, mtd);
  	count = mtd_div_by_eb(instr->len, mtd);
-	gluebi = container_of(mtd, struct gluebi_device, mtd);

  	for (i = 0; i < count - 1; i++) {
  		err = ubi_leb_unmap(gluebi->desc, lnum + i);
@@ -259,10 +295,14 @@ static int gluebi_erase(struct mtd_info *mtd, 
struct erase_info *instr)
  	if (err)
  		goto out_err;

+	if (not_get)
+		__put_mtd_device(mtd);
  	return 0;

  out_err:
  	instr->fail_addr = (long long)lnum * mtd->erasesize;
+	if (not_get)
+		__put_mtd_device(mtd);
  	return err;
  }

-- 
2.31.1

Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[Zhihao Cheng]

在 2023/10/11 10:32, ZhaoLong Wang 写道:
> 
>> This patch assumes that the gluebi module is not designed to work with
>> the ftl module. In this case, the patch only needs to prevent the ftl
>> notifier operation.
>>
>> Add some correctness check for gluebi->desc in gluebi_read/write/erase(),
>> If the pointer is invalid, the -EINVAL is returned.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
>> Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
>> ---
>>   drivers/mtd/ubi/gluebi.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
>> index 1b980d15d9fb..189ecc0eacd1 100644
>> --- a/drivers/mtd/ubi/gluebi.c
>> +++ b/drivers/mtd/ubi/gluebi.c
>> @@ -157,6 +157,9 @@ static int gluebi_read(struct mtd_info *mtd, 
>> loff_t from, size_t len,
>>       struct gluebi_device *gluebi;
>>       gluebi = container_of(mtd, struct gluebi_device, mtd);
>> +    if (IS_ERR_OR_NULL(gluebi->desc))
>> +        return -EINVAL;
>> +
>>       lnum = div_u64_rem(from, mtd->erasesize, &offs);
>>       bytes_left = len;
>>       while (bytes_left) {
>> @@ -197,6 +200,9 @@ static int gluebi_write(struct mtd_info *mtd, 
>> loff_t to, size_t len,
>>       struct gluebi_device *gluebi;
>>       gluebi = container_of(mtd, struct gluebi_device, mtd);
>> +    if (IS_ERR_OR_NULL(gluebi->desc))
>> +        return -EINVAL;
>> +
>>       lnum = div_u64_rem(to, mtd->erasesize, &offs);
>>       if (len % mtd->writesize || offs % mtd->writesize)
>> @@ -242,6 +248,8 @@ static int gluebi_erase(struct mtd_info *mtd, 
>> struct erase_info *instr)
>>       lnum = mtd_div_by_eb(instr->addr, mtd);
>>       count = mtd_div_by_eb(instr->len, mtd);
>>       gluebi = container_of(mtd, struct gluebi_device, mtd);
>> +    if (IS_ERR_OR_NULL(gluebi->desc))
>> +        return -EINVAL;
>>       for (i = 0; i < count - 1; i++) {
>>           err = ubi_leb_unmap(gluebi->desc, lnum + i);
> 
> 
> This modification attempts another solution. Always check the validity
> of gluebi->desc. If the gluebi->desc pointer is invalid, try to get MTD
> device.
> 
> 
> diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
> index 1b980d15d9fb..f1a74ccf1718 100644
> --- a/drivers/mtd/ubi/gluebi.c
> +++ b/drivers/mtd/ubi/gluebi.c
> @@ -154,9 +154,19 @@ 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 not_get = IS_ERR_OR_NULL(gluebi->desc);
> +
> +    if (not_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 +186,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t 
> from, size_t len,
>       }
> 
>       *retlen = len - bytes_left;
> +
> +    if (not_get)
> +        __put_mtd_device(mtd);
>       return err;
>   }
> 

I'm afraid that this patch won't cover following three situations 
completely:
1. gluebi_create -> ftl_add_mtd -> mtd_read -> gluebi_read:
    gluebi->desc is NULL.       (√)
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       (√)
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  (×)

> @@ -194,9 +207,19 @@ static int gluebi_write(struct mtd_info *mtd, 
> loff_t to, size_t len,
>               size_t *retlen, const u_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 not_get = IS_ERR_OR_NULL(gluebi->desc);
> +
> +    if (not_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(to, mtd->erasesize, &offs);
> 
>       if (len % mtd->writesize || offs % mtd->writesize)
> @@ -220,6 +243,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t 
> to, size_t len,
>       }
> 
>       *retlen = len - bytes_left;
> +
> +    if (not_get)
> +        __put_mtd_device(mtd);
>       return err;
>   }
> 
> @@ -234,14 +260,24 @@ static int gluebi_write(struct mtd_info *mtd, 
> loff_t to, size_t len,
>   static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr)
>   {
>       int err, i, lnum, count;
> -    struct gluebi_device *gluebi;
> +    struct gluebi_device *gluebi = container_of(mtd, struct gluebi_device,
> +                            mtd);
> +    int not_get = IS_ERR_OR_NULL(gluebi->desc);
> +
> +    if (not_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;
> +        }
> +    }
> 
>       if (mtd_mod_by_ws(instr->addr, mtd) || mtd_mod_by_ws(instr->len, 
> mtd))
>           return -EINVAL;
> 
>       lnum = mtd_div_by_eb(instr->addr, mtd);
>       count = mtd_div_by_eb(instr->len, mtd);
> -    gluebi = container_of(mtd, struct gluebi_device, mtd);
> 
>       for (i = 0; i < count - 1; i++) {
>           err = ubi_leb_unmap(gluebi->desc, lnum + i);
> @@ -259,10 +295,14 @@ static int gluebi_erase(struct mtd_info *mtd, 
> struct erase_info *instr)
>       if (err)
>           goto out_err;
> 
> +    if (not_get)
> +        __put_mtd_device(mtd);
>       return 0;
> 
>   out_err:
>       instr->fail_addr = (long long)lnum * mtd->erasesize;
> +    if (not_get)
> +        __put_mtd_device(mtd);
>       return err;
>   }
> 

No need to modify 'gluebi_write' and 'gluebi_erase'.

Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[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.

> 
> 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;
  }

Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[Zhihao Cheng]

在 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;
>   }
> 
> 
> 
> .

Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[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.


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
                   mutex_lock(&mtd_table_mutex);
                   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
                   mutex_unlock(&mtd_table_mutex);

ubi_cdev_ioctl
   ubi_create_volume
     ubi_volume_notify
       blocking_notifier_call_chain  [kernel/notifier.c]
         notifier_call_chain
           gluebi_notify   nb->notifier_call()
             gluebi_create
               mtd_device_register
                 mtd_device_parse_register
                   add_mtd_device
                     mutex_lock(&mtd_table_mutex);
                     blktrans_notify_add   not->add()
                       ftl_add_mtd     tr->add_mtd()
                         scan_header
                           mtd_read(part->mbd.mtd,
                             mtd_read_oob
                               gluebi_read   mtd->read()
                                 ubi_read(gluebi->desc
                     mutex_unlock(&mtd_table_mutex);

load_module ftl
   register_mtd_blktrans
     mutex_lock(&mtd_table_mutex);
     ftl_add_mtd  not->add()
       scan_header
         mtd_read(part->mbd.mtd,
           mtd_read_oob
             gluebi_read   mtd->read()
               ubi_read(gluebi->desc
     mutex_unlock(&mtd_table_mutex);

This lock is also held during the process of closing the file.

close(fd)
   mtdchar_close
     put_mtd_device
       mutex_lock(&mtd_table_mutex);
       __put_mtd_device
         gluebi_put_device
           ubi_close_volume
             kfree(desc)
             // desc == NULL
       mutex_unlock(&mtd_table_mutex);

Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[Zhihao Cheng]

在 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;
 > +        }
 > +    }

Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[Zhihao Cheng]

在 2023/10/10 22:29, ZhaoLong Wang 写道:
> If both flt.ko and gluebi.ko are loaded, the notiier of ftl
> triggers NULL pointer dereference when trying to visit
> ‘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.
> 
> This patch assumes that the gluebi module is not designed to work with
> the ftl module. In this case, the patch only needs to prevent the ftl
> notifier operation.

We can't refuse ftl when gluebi is loaded, it looks weird:
mtd0(nand) -> ftl0
mtd1(gluebi) has no ftl1?

When FTL is loaded, it should make sure each mtd device correspond to a 
block device, no matter which type the mtd device is(nand or gluebi).

So I prefer the root cause is gluebi->desc is not initialized in 
gluebi_create->mtd_device_register.

> 
> Add some correctness check for gluebi->desc in gluebi_read/write/erase(),
> If the pointer is invalid, the -EINVAL is returned.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
> Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
> ---
>   drivers/mtd/ubi/gluebi.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
> index 1b980d15d9fb..189ecc0eacd1 100644
> --- a/drivers/mtd/ubi/gluebi.c
> +++ b/drivers/mtd/ubi/gluebi.c
> @@ -157,6 +157,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len,
>   	struct gluebi_device *gluebi;
>   
>   	gluebi = container_of(mtd, struct gluebi_device, mtd);
> +	if (IS_ERR_OR_NULL(gluebi->desc))
> +		return -EINVAL;
> +
>   	lnum = div_u64_rem(from, mtd->erasesize, &offs);
>   	bytes_left = len;
>   	while (bytes_left) {
> @@ -197,6 +200,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len,
>   	struct gluebi_device *gluebi;
>   
>   	gluebi = container_of(mtd, struct gluebi_device, mtd);
> +	if (IS_ERR_OR_NULL(gluebi->desc))
> +		return -EINVAL;
> +
>   	lnum = div_u64_rem(to, mtd->erasesize, &offs);
>   
>   	if (len % mtd->writesize || offs % mtd->writesize)
> @@ -242,6 +248,8 @@ static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr)
>   	lnum = mtd_div_by_eb(instr->addr, mtd);
>   	count = mtd_div_by_eb(instr->len, mtd);
>   	gluebi = container_of(mtd, struct gluebi_device, mtd);
> +	if (IS_ERR_OR_NULL(gluebi->desc))
> +		return -EINVAL;
>   
>   	for (i = 0; i < count - 1; i++) {
>   		err = ubi_leb_unmap(gluebi->desc, lnum + i);
> 

Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[ZhaoLong Wang]

Friendly ping ...

> If both flt.ko and gluebi.ko are loaded, the notiier of ftl
> triggers NULL pointer dereference when trying to visit
> ‘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.