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