| Message ID | 20231124113037.2477645-1-frolov@swemel.ru |
|---|---|
| State | New |
| Headers | show |
| Series | block/monitor: blk_bs() return value check | expand |
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
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 >
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
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);
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(-)