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 <[email protected]>
---
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
> 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 <[email protected]>
> ---
> 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
在 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 <[email protected]>
> ---
> 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);
>
在 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 <[email protected]>
>> ---
>> 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'.
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;
}
在 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;
> }
>
>
>
> .
>>
>>> 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);
在 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;
> + }
> + }
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.