|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH for-6.0] block/nbd: fix possible use after free of s->connect_thread |
Date: | Tue, 13 Apr 2021 15:19:56 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 |
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; + } + qemu_mutex_lock(&thr->mutex); switch (thr->state) {First, it is a bit strange not to set *errp in these cases.
Oops, right! ashamed)
I don’t think it’d make a difference anyway, but now that I look into it... The error would be propagated to s->connect_err, but that isn’t used anywhere, so... What’s the point?
Yes it's unused.. That's to be improved later.
Anyway. What I really wanted to ask is: nbd_co_establish_connection() may create a thread, which accesses the thread. Is that a problem? Like, can this happen: - nbd_co_establish_connection(): thr->state := THREAD_RUNNING - nbd_co_establish_connection(): thread is created - nbd_co_establish_connection(): thr->mutex unlocked - connect_thread_func(): thr->mutex locked - connect_thread_func(): thr->state := something else - connect_thread_func(): thr->mutex unlocked - nbd_co_establish_connection(): yields - (As I understood it, this yield leads to nbd_co_establish_connection_cancel() continuing) - nbd_co_EC_cancel(): thr->mutex locked - nbd_co_EC_cancel(): do_free true - nbd_co_EC_cancel(): nbd_free_connect_thread() - connect_thread_func(): nbd_free_connect_thread()
I think not. Here we are safe: connect_thread_func will only do free if thread in CONNECT_THREAD_RUNNING_DETACHED state. Thread becomes CONNECT_THREAD_RUNNING_DETACHED only on one code path in nbd_co_establish_connection_cancel(), and on that path do_free is false. OK, what you say is possible if we call nbd_co_establish_connection_cancel() twice with detach=true. But that should not be possible if it is called only from .bdrv_close (indirectly) of nbd driver. The problem is that nbd_co_EC_cancel() may free the thread state, and then nbd_co_establish_connection() reuses saved thr local varible after yield. Still (as I noted before), I've never reproduced it on master, only on my branch with some modifications. Still it seems possible anyway. -- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |