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

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Dec 12, 2018, at 3:45 PM, Ian Jackson <ian.jackson@xxxxxxxxxx> wrote:
> 
> George Dunlap writes ("[PATCH v2 05/10] libxl: Do root checks once in 
> libxl__domain_get_device_model_uid"):
>> At the moment, we check for equivalence to literal "root" before
>> deciding whether to add the `runas` command-line option to QEMU.  This
>> is unsatisfactory for several reasons.
> ...
>> v2:
>> - Refactor to use `out` rather than multiple labels
>> - Only check for root once
>> - Use 'out' rather than direct returns for errors (only use direct returns
>>  for early `succeed-without-setting-runas` paths)
>> - Use `rc` rather than `ret` to more closely align with CODING_STYLE
>> - Fill out comments about the cases we're handling
>> - Return ERROR_DEVICE_EXISTS rather than ERROR_FAIL if there's another
>>  username that maps to our calculated uid
>> - Report an error if the specified device_model_user doesn't exist
> 
> Thanks.  This is all much better now.

FWIW I agree. :-)

> Or you could treat the !user_base as an error block and write this:
> 
>> +        if (!user_base) {
>> +            LOGD(ERROR, guest_domid, "Couldn't find device_model_user %s",
>> +                 user);
>> +            rc = ERROR_INVAL;
>  +            goto out;
>  +        }
>  +        intended_uid = user_base->pw_uid;
>  +        rc = 0;
>  +        goto out;
>> +    }

This looks good.

> 
>> +    /*
>> +     * 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.

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

> 
>>         if (user_clash) {
>>             LOGD(ERROR, guest_domid,
>>                  "wanted to use uid %ld (%s + %d) but that is user %s !",
>>                  (long)intended_uid, LIBXL_QEMU_USER_RANGE_BASE,
>>                  guest_domid, user_clash->pw_name);
>> -            return ERROR_FAIL;
>> +            rc = ERROR_DEVICE_EXISTS;
>> +            goto out;
> 
> I appreciate your desire to specify particular error value, but I am
> far from convinced that ERROR_DEVICE_EXISTS is appropriate.  It would
> lead someone to ask which device was in the way.

Consider this patch the most concise way of asking the question, “What error 
should I use in this case?” :-)

> 
> 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. :-)

I’ll think about whether to introduce a new one or just use ERROR_INVAL.

> 
>> +    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

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]

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’ll write redundant statements everywhere if you want, but I thought that 
would count as the kind of code duplication you wanted to avoid.

> 
>> +out:
>> +    if (!rc) {
>> +        if (intended_uid == 0) {
>> +            LOGD(ERROR, guest_domid, "intended_uid is 0 (root)!");
>> +            return ERROR_INVAL;
>> +        }
>> +
>> +        state->dm_runas = user;
>> +    }
>> +
>> +    return rc;
> 
> TBH I dislike the early return in the `goto out' block.  If a resource
> deallocation were to be introduced, that `return ERROR_INVAL' would
> become a memory leak.
> 
> 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.

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

if (!rc && user) {
 /* check */
}
if (!rc && user) {
}

or 

if (user) {
  if (!rc) {
    /* check */
  }
  if (!rc) {
    /* set */
  }
}

or of course:

if (!rc && user) {
  if (/*check*) {
    rc = ERROR_INVAL;
  } else {
    /* set */
  }
}

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

 -George

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