lkml.org 
[lkml]   [2023]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier
    From
    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;
    }


    \
     
     \ /
      Last update: 2023-10-12 10:04    [W:2.467 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site