diff mbox series

[2/8] block/export: Fix null pointer dereference in error path

Message ID 20230510203601.418015-3-kwolf@redhat.com (mailing list archive)
State New, archived
Headers show
Series block: Honour graph read lock even in the main thread | expand

Commit Message

Kevin Wolf May 10, 2023, 8:35 p.m. UTC
There are some error paths in blk_exp_add() that jump to 'fail:' before
'exp' is even created. So we can't just unconditionally access exp->blk.

Add a NULL check, and switch from exp->blk to blk, which is available
earlier, just to be extra sure that we really cover all cases where
BlockDevOps could have been set for it (in practice, this only happens
in drv->create() today, so this part of the change isn't strictly
necessary).

Fixes: de79b52604e43fdeba6cee4f5af600b62169f2d2
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/export/export.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Peter Maydell May 12, 2023, 3:31 p.m. UTC | #1
On Wed, 10 May 2023 at 21:38, Kevin Wolf <kwolf@redhat.com> wrote:
>
> There are some error paths in blk_exp_add() that jump to 'fail:' before
> 'exp' is even created. So we can't just unconditionally access exp->blk.
>
> Add a NULL check, and switch from exp->blk to blk, which is available
> earlier, just to be extra sure that we really cover all cases where
> BlockDevOps could have been set for it (in practice, this only happens
> in drv->create() today, so this part of the change isn't strictly
> necessary).
>
> Fixes: de79b52604e43fdeba6cee4f5af600b62169f2d2
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

Coverity noticed this bug, incidentally: CID 1509238.

-- PMM
Eric Blake May 12, 2023, 4:16 p.m. UTC | #2
On Wed, May 10, 2023 at 10:35:55PM +0200, Kevin Wolf wrote:
> 
> There are some error paths in blk_exp_add() that jump to 'fail:' before
> 'exp' is even created. So we can't just unconditionally access exp->blk.
> 
> Add a NULL check, and switch from exp->blk to blk, which is available
> earlier, just to be extra sure that we really cover all cases where
> BlockDevOps could have been set for it (in practice, this only happens
> in drv->create() today, so this part of the change isn't strictly
> necessary).
> 
> Fixes: de79b52604e43fdeba6cee4f5af600b62169f2d2

Sorry for missing that on my first review, and this does look better.

I'm assuming you plan to take this in with the rest of the series
through your tree, but let me know if I should push it faster through
the NBD tree.

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/export/export.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/export/export.c b/block/export/export.c
> index 62c7c22d45..a5c8f42f53 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -192,8 +192,10 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>      return exp;
>  
>  fail:
> -    blk_set_dev_ops(exp->blk, NULL, NULL);
> -    blk_unref(blk);
> +    if (blk) {
> +        blk_set_dev_ops(blk, NULL, NULL);
> +        blk_unref(blk);
> +    }
>      aio_context_release(ctx);
>      if (exp) {
>          g_free(exp->id);
> -- 
> 2.40.1
> 
>
Eric Blake May 12, 2023, 6:46 p.m. UTC | #3
On Fri, May 12, 2023 at 11:16:03AM -0500, Eric Blake wrote:
> 
> 
> On Wed, May 10, 2023 at 10:35:55PM +0200, Kevin Wolf wrote:
> > 
> > There are some error paths in blk_exp_add() that jump to 'fail:' before
> > 'exp' is even created. So we can't just unconditionally access exp->blk.
> > 
> > Add a NULL check, and switch from exp->blk to blk, which is available
> > earlier, just to be extra sure that we really cover all cases where
> > BlockDevOps could have been set for it (in practice, this only happens
> > in drv->create() today, so this part of the change isn't strictly
> > necessary).
> > 
> > Fixes: de79b52604e43fdeba6cee4f5af600b62169f2d2
> 
> Sorry for missing that on my first review, and this does look better.
> 
> I'm assuming you plan to take this in with the rest of the series
> through your tree, but let me know if I should push it faster through
> the NBD tree.

Because iotest:
./check 307 -nbd

fails without this patch but passes with it,

Tested-by: Eric Blake <eblake@redhat.com>

[and I should really remember to run more iotests than just the subset
run by 'make check' when preparing a pull request...]
diff mbox series

Patch

diff --git a/block/export/export.c b/block/export/export.c
index 62c7c22d45..a5c8f42f53 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -192,8 +192,10 @@  BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
     return exp;
 
 fail:
-    blk_set_dev_ops(exp->blk, NULL, NULL);
-    blk_unref(blk);
+    if (blk) {
+        blk_set_dev_ops(blk, NULL, NULL);
+        blk_unref(blk);
+    }
     aio_context_release(ctx);
     if (exp) {
         g_free(exp->id);