qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err
Date: Wed, 25 Mar 2020 07:28:02 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

24.03.2020 23:03, Eric Blake wrote:
On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:
local_err is used several times in guest_suspend(). Setting non-NULL
local_err will crash, so let's zero it after freeing. Also fix possible
leak of local_err in final if().

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
  qga/commands-posix.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 93474ff770..cc69b82704 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
      }
      error_free(local_err);
+    local_err = NULL;

Let's show this with more context.

static void guest_suspend(SuspendMode mode, Error **errp)
{
    Error *local_err = NULL;
    bool mode_supported = false;

    if (systemd_supports_mode(mode, &local_err)) {

Hmm - we have an even earlier bug that needs fixing.  Note that systemd_supports_mode() returns a 
bool AND conditionally sets errp.  But it is inconsistent: it has the following table of actions 
based on the results of run_process_child() on "systemctl status" coupled with the man 
page on "systemctl status" return values:
-1 (unable to run systemctl) -> errp set, return false
0 (unit is active) -> errp left unchanged, return false
1 (unit not failed) -> errp left unchanged, return true
2 (unused) -> errp left unchanged, return true
3 (unit not active) -> errp left unchanged, return true
4 (no such unit) -> errp left unchanged, return false
5+ (unexpected from systemctl) -> errp left unchanged, return false

But the comments in systemd_supports_mode() claim that ANY status < 4 (other 
than -1, which means we did not run systemctl) should count as the service 
existing, even though the most common status is 3.  If our comment is to be 
believed, then we should return true, not false, for status 0.

Now, back to _this_ function:

        mode_supported = true;
        systemd_suspend(mode, &local_err);

Okay - if we get here (whether from status 1-3, or with systemd_supports_mode 
fixed to support status 0-3), local_err is still unset prior to calling 
systemd_suspend(), and we are guaranteed that after the call, either we 
suspended successfully or local_err is now set.

    }

    if (!local_err) {
        return;
    }

So if returned, we succeeded at systemd_suspend, and there is nothing further 
to do; but if we get past that point, we don't know if it was 
systemd_supports_mode that failed or systemd_suspend that failed, and we don't 
know if local_err is set.

No, we know that is set, as we check exactly this and return if not set.



    error_free(local_err);
+    local_err = NULL;

Yet, we blindly throw away local_err, without trying to report it.  If that's 
the case, then WHY are we passing in local_err?  Wouldn't it be better to pass 
in NULL (we really don't care about the error message), and/or fix 
systemd_suspend() to return a bool just like systemd_supports_mode, and/or fix 
systemd_supports_mode to guarantee that it sets errp when returning false?


I agree that this is a strange function and its logic is weird. But I don't 
know what the logic should be. My patch is still valid to just fix obvious 
use-after-free and possible leak. It doesn't fix the logic.


--
Best regards,
Vladimir



reply via email to

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