[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH for-6.0] block/nbd: fix possible use after free of s->connect

From: Max Reitz
Subject: Re: [PATCH for-6.0] block/nbd: fix possible use after free of s->connect_thread
Date: Tue, 13 Apr 2021 15:32:39 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0

On 13.04.21 14:19, Vladimir Sementsov-Ogievskiy wrote:
13.04.2021 14:53, Max Reitz wrote:
On 06.04.21 17:51, Vladimir Sementsov-Ogievskiy wrote:
If on nbd_close() we detach the thread (in
nbd_co_establish_connection_cancel() thr->state becomes
CONNECT_THREAD_RUNNING_DETACHED), after that point we should not use
s->connect_thread (which is set to NULL), as running thread may free it
at any time.

Still nbd_co_establish_connection() does exactly this: it saves
s->connect_thread to local variable (just for better code style) and
use it even after yield point, when thread may be already detached.

Fix that. Also check thr to be non-NULL on
nbd_co_establish_connection() start for safety.

After this patch "case CONNECT_THREAD_RUNNING_DETACHED" becomes
impossible in the second switch in nbd_co_establish_connection().
Still, don't add extra abort() just before the release. If it somehow
possible to reach this "case:" it won't hurt. Anyway, good refactoring
of all this reconnect mess will come soon.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Hi all! I faced a crash, just running 277 iotest in a loop. I can't
reproduce it on master, it reproduces only on my branch with nbd
reconnect refactorings.

Still, it seems very possible that it may crash under some conditions.
So I propose this patch for 6.0. It's written so that it's obvious that
it will not hurt:

  pre-patch, on first hunk we'll just crash if thr is NULL,
  on second hunk it's safe to return -1, and using thr when
  s->connect_thread is already zeroed is obviously wrong.

  block/nbd.c | 11 +++++++++++
  1 file changed, 11 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index c26dc5a54f..1d4668d42d 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -443,6 +443,11 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
      BDRVNBDState *s = bs->opaque;
      NBDConnectThread *thr = s->connect_thread;
+    if (!thr) {
+        /* detached */
+        return -1;
+    }
      switch (thr->state) {

First, it is a bit strange not to set *errp in these cases.

Oops, right! ashamed)

OK, so who cares.  It wouldn’t do anything anyway.

Apart from that, all the changes do is to turn use after frees or immediate NULL dereferences into clean errors. I can’t see any resources that should be cleaned up, so I hope Coverity won’t hate me for taking this patch.

And then we’ll see whether Peter will take the pull request...


reply via email to

[Prev in Thread] Current Thread [Next in Thread]