Re: [Xen-devel] [PATCH v2 05/10] libxl: Do root checks once in libxl__domain_get_device_model_uid

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

George Dunlap writes ("Re: [PATCH v2 05/10] libxl: Do root checks once in 
libxl__domain_get_device_model_uid"):
> On Dec 12, 2018, at 3:45 PM, Ian Jackson <ian.jackson@xxxxxxxxxx> wrote:
> >> +    /*
> >> +     * If dm_restrict isn't set, and we don't have a specified user, don't
> >> +     * bother setting a `-runas` parameter.
> >> +     */
> >>     if (!libxl_defbool_val(b_info->dm_restrict)) {
> >>         LOGD(DEBUG, guest_domid,
> >>              "dm_restrict disabled, starting QEMU as root");
> >>         return 0;
> >>     }
> > 
> > Why `return 0' here but `goto out' earlier ?  IMO all the success
> > returns should be the same.
> 
> Not exactly — we only want to do the root check if we’re running as an 
> alternate user.  In this case, neither device_model_user nor dm_restrict is 
> set, so we’re not running as an alternate user, so we don’t want to run the 
> `if(intended_uid == 0)` check on the normal ‘out’ path.

Ah.  Fiddly.

> I take it you’d prefer to always jump to out, but to have the conditional be, 
> `if (!rc && user)`?

How about
  if (!rc) {
     if (user && intended_uid=0) {
         error case,
         rc = 
     }
  }
  if (!rc) {
?

The reason I like this is because this keeps separate the error / flow
control logic from the precise details of the reason why we might
discover a later error.

> > We generally use EINVAL for bad configurations.  If you prefer, feel
> > free to introduce a new error code.  32-bit signed integers are pretty
> > cheap.
> 
> The integers may be cheap, but scanning through trying to comprehend them is 
> not. :-)

It would be nice if we had a doc saying what they meant.

> >> +    if (rc < 0)
> >> +        goto out;
> >>     if (user_base) {
> >>         LOGD(WARN, guest_domid, "Could not find user %s, falling back to 
> >> %s",
> >>              LIBXL_QEMU_USER_RANGE_BASE, LIBXL_QEMU_USER_SHARED);
> >> -        goto end_search;
> >> +        intended_uid = user_base->pw_uid;
> >> +        goto out;
> > 
> > Here we have this pattern again with a `goto out' without a preceding
> > assignment to `rc'.  AFAICT the rules implied by your out block are:
> > 
> > * Every goto out must be preceded by an assignment to rc.
> >   IMO there is no reason this should not immediately precede
> >   the goto out.
> > 
> > * Additionally, if rc is 0 then the goto out must also be preceded
> >   relatively recently by an assignment to intended_uid.
> 
> Those are rules that you’re implying, not me. :-)  My `goto out` invariant in 
> this patch were:
> 
> 1. rc may be an error code.  In this case, rc is returned.
> 2. rc may be zero; if rc is zero:
>  2a. user must be non-NULL,
>  2b. user must be verified to exist on the system, and
>  2c. intended_uid must be set to the userid reported in the previous check

This formulation of the rules cannot be verified locally.  In order to
verify these for any particular `goto out' it is necessary to scan
back through the previous logic to see whether 2a, 2b, 2c are true.

But you are right that I had failed to spot that assignment to user is
also required.  Perhaps this demonstrates that a comment stating the
rules explicitly would be useful.

> In order to accept your suggestion above to replace the `return` with a `goto 
> out`, I have to make the invariant as follows:
> 
> 1. rc may be an error code.  In this case, rc is returned.
> 2. rc may be zero, and user NULL.  In this case, rc is returned.
> 3. rc may be zero, and user non-NULL.  In this case:
>   [2b and 2c from above]

I would suggest:

  1. If rc is an error code, all bets are off about user and
     intended_uid

  2. Otherwise we have success.  Then user and intended_uid are
     appropriate for the situation, which might mean both are
     sentinel, or both are set to specific values.

This seems to make sense to me since the purpose of this function is
to discover the right values for user and intended_uid.

> In this case, we know that rc is 0 because we just checked the value 6 lines 
> earlier.  If code is ever added in between such that rc becomes non-zero, 
> *that* code should be calling `goto out` (or thinking carefully about why 
> falling through to this code is OK).

I really dislike the pattern of reusing an rc value.  It is OK to
build up the results of the computation in the intended answer
variables (in this case user and intended_uid).

But rc is a `throwaway' variable which has to be trashed by any call
to any libxl subfunction.  That is, except in the special case of
destruction functions, and during the out block, rc is not accumulated
or preserved, and has only very local significance.

You are saying `but if that other subfunction doesn't succeed, setting
rc!=0, it will have to goto out' but that is not a valid assumption.
Maybe there is a way of handling the error by trying an alternative
approach, or something.  In general there is nothing wrong with code
like this:

   rc = libxl_try_mutilate_wombat(gc, dom, &wombat);
   /* We do not need to mutilate a nonexistent wombat */
   if (rc && rc != ERROR_NOWOMBAT)
       goto out;

That is the standard pattern for making a subroutine call, adjusted
for ignoring an expected harmless error case.  Unless there is a good
reason to do otherwise, code in libxl should tolerate having something
like that dropped into it, in between other work.

But of course the fragment above does not leave rc set to anything in
particular.  If subsequent code wants rc==0 it would have to set it.

This implies that unless there is a good reason to do otherwise, all
code in libxl that wants rc==0 should set it.  Even if the preceding
little fragment of code is, at present, a necessarily-successful
subroutine call.

> I’ll write redundant statements everywhere if you want, but I thought that 
> would count as the kind of code duplication you wanted to avoid.

The reason why code duplication is bad is that duplicated code, like
all code, has bugs, and fixing the bugs is then duplicated too.  This
hardly applies to `rc = 0;' before `goto out;'.  Rather, I regard
reliance on some previous rc a latent bug.

> > I think this means lifting state->dm_runas into else, or writing this:
> > 
> >> +out:
> >> +    if (!rc) {
> >> +        if (intended_uid == 0) {
> >> +            LOGD(ERROR, guest_domid, "intended_uid is 0 (root)!");
> >  +            rc = ERROR_INVAL;
> >> +        }
> >  +    }
> >  +    if (!rc) {
> >> +        state->dm_runas = user;
> >> +    }
> 
> Would you want braces on the second `if`?  I suppose we’d add them in patch 7 
> anyway.

Yes.  That's why I wrote them there :-).

> If we switch the earlier `return 0` in the !dm_restrict conditional to a 
> “goto out”, then this would turn into:

I think setting intended_uid==0 when user==0 is a hostage to fortune.
Why not set it to (uid_t)-1 ?

Then you write:

  if (!rc) {
     if (intended_uid == 0) {
         complain
         rc = ERROR_INVAL;
     }
  }
  if (!rc) {
     save user and intended_uid
  }

> If you have a favorite color it might be better just to tell me. :-)

Do you see why I prefer the above ?

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel