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

[Vladimir Sementsov-Ogievskiy]

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