[PATCH] block/monitor: blk_bs() return value check

[PATCH] block/monitor: blk_bs() return value check

[Dmitry Frolov]

blk_bs() may return NULL, which will be dereferenced without a check in
bdrv_commit().

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
 block/monitor/block-hmp-cmds.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index c729cbf1eb..ade627bc27 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -221,7 +221,13 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
             return;
         }
 
-        bs = bdrv_skip_implicit_filters(blk_bs(blk));
+        bs = blk_bs(blk);
+        if (!bs) {
+            error_report("Device '%s' is invalid", device);
+            return;
+        }
+
+        bs = bdrv_skip_implicit_filters(bs);
         aio_context = bdrv_get_aio_context(bs);
         aio_context_acquire(aio_context);
 
-- 
2.34.1

Re: [PATCH] block/monitor: blk_bs() return value check

[Kevin Wolf]

Am 24.11.2023 um 12:30 hat Dmitry Frolov geschrieben:
> blk_bs() may return NULL, which will be dereferenced without a check in
> bdrv_commit().
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>

Do you have a reproducer for a crash?

As far as I can see, it will not be dereferenced, because
blk_is_available() returns false and we return an error before
calling bdrv_commit():

    QEMU 8.1.91 monitor - type 'help' for more information
    (qemu) info block
    ide1-cd0: [not inserted]
        Attached to:      /machine/unattached/device[6]
        Removable device: not locked, tray closed

    floppy0: [not inserted]
        Attached to:      /machine/unattached/device[16]
        Removable device: not locked, tray closed

    sd0: [not inserted]
        Removable device: not locked, tray closed
    (qemu) commit ide1-cd0 
    Device 'ide1-cd0' has no medium

Kevin

Re: [PATCH] block/monitor: blk_bs() return value check

[Дмитрий Фролов]

On 24.11.2023 16:06, Kevin Wolf wrote:
> Am 24.11.2023 um 12:30 hat Dmitry Frolov geschrieben:
>> blk_bs() may return NULL, which will be dereferenced without a check in
>> bdrv_commit().
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> Do you have a reproducer for a crash?
Actually, there was no crash. This problem was found by static analyzer.
> As far as I can see, it will not be dereferenced, because
> blk_is_available() returns false and we return an error before
> calling bdrv_commit():
As I see, there are 2 reasons, why blk_bs() may return NULL:
blk->root == NULL or blk->root->bs == NULL
At the same time, blk_is_available() checks for blk_co_is_inserted(blk) and
blk_dev_is_tray_open(blk).
Does it also guarantee that blk->root and blk->root->bs are not NULL?
This is not really obvious.

Maybe, in this case, it makes sense to check blk->root before of checking
blk_is_available()?
>      QEMU 8.1.91 monitor - type 'help' for more information
>      (qemu) info block
>      ide1-cd0: [not inserted]
>          Attached to:      /machine/unattached/device[6]
>          Removable device: not locked, tray closed
>
>      floppy0: [not inserted]
>          Attached to:      /machine/unattached/device[16]
>          Removable device: not locked, tray closed
>
>      sd0: [not inserted]
>          Removable device: not locked, tray closed
>      (qemu) commit ide1-cd0
>      Device 'ide1-cd0' has no medium
>
> Kevin
>

Re: [PATCH] block/monitor: blk_bs() return value check

[Kevin Wolf]

Am 24.11.2023 um 15:05 hat Дмитрий Фролов geschrieben:
> 
> 
> On 24.11.2023 16:06, Kevin Wolf wrote:
> > Am 24.11.2023 um 12:30 hat Dmitry Frolov geschrieben:
> > > blk_bs() may return NULL, which will be dereferenced without a check in
> > > bdrv_commit().
> > > 
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > > 
> > > Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> > Do you have a reproducer for a crash?
> Actually, there was no crash. This problem was found by static analyzer.
> > As far as I can see, it will not be dereferenced, because
> > blk_is_available() returns false and we return an error before
> > calling bdrv_commit():
> As I see, there are 2 reasons, why blk_bs() may return NULL:
> blk->root == NULL or blk->root->bs == NULL

blk->root->bs == NULL shouldn't happen, but the code we're looking at is
safe even for this case.

> At the same time, blk_is_available() checks for
> blk_co_is_inserted(blk) and blk_dev_is_tray_open(blk).
> Does it also guarantee that blk->root and blk->root->bs are not NULL?
> This is not really obvious.

blk_co_is_inserted() does, it returns false for blk_bs(blk) == NULL.

> Maybe, in this case, it makes sense to check blk->root before of
> checking blk_is_available()?

Checking blk->root and those few other things is a really common thing
that most operations do, which is why we have blk_is_available() to
check all of this. If we did the checks before calling it, we wouldn't
need blk_is_available() any more.

Kevin