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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2d04fa9e-e594-705c-339b-3090cb7d6fbd@huawei.com>
Date:   Thu, 12 Oct 2023 17:31:11 +0800
From:   ZhaoLong Wang <wangzhaolong1@...wei.com>
To:     Zhihao Cheng <chengzhihao1@...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


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