Re: [PATCH 4/4] xen/version: Introduce non-truncating XENVER_* subops

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 04/01/2023 5:04 pm, Jan Beulich wrote:
> On 03.01.2023 21:09, Andrew Cooper wrote:
>> Recently in XenServer, we have encountered problems caused by both
>> XENVER_extraversion and XENVER_commandline having fixed bounds.
>>
>> More than just the invariant size, the APIs/ABIs also broken by typedef-ing 
>> an
>> array, and using an unqualified 'char' which has implementation-specific
>> signed-ness.
> Which is fine as long as only ASCII is returned. If non-ASCII can be returned,
> I agree "unsigned char" is better, but then we also need to spell out what
> encoding the strings use (UTF-8 presumably).

Well, it "functions" as far as ASCII is concerned, but I wouldn't say
that was fine for an ABI.

The command line can reasonably have non-ASCII characters these days. 
(as can the compile information, but I intentionally didn't convert them
to this new format).

UTF-8 is probably the sensible thing to specify, if we're going to make
any statement.

>
>> API/ABI wise, XENVER_build_id could be merged into xenver_varstring_op(), but
>> the internal infrastructure is awkward.
> I guess build-id also doesn't fit the rest because of not returning strings,
> but indeed an array of bytes. You also couldn't use strlen() on the array.

The format is unspecified, but it is a base64 encoded ASCII string (not
NUL terminated).

>> @@ -469,6 +470,66 @@ static int __init cf_check param_init(void)
>>  __initcall(param_init);
>>  #endif
>>  
>> +static long xenver_varstring_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> +{
>> +    const char *str = NULL;
>> +    size_t sz = 0;
>> +    union {
>> +        xen_capabilities_info_t info;
>> +    } u;
>> +    struct xen_var_string user_str;
>> +
>> +    switch ( cmd )
>> +    {
>> +    case XENVER_extraversion2:
>> +        str = xen_extra_version();
>> +        break;
>> +
>> +    case XENVER_changeset2:
>> +        str = xen_changeset();
>> +        break;
>> +
>> +    case XENVER_commandline2:
>> +        str = saved_cmdline;
>> +        break;
>> +
>> +    case XENVER_capabilities2:
>> +        memset(u.info, 0, sizeof(u.info));
>> +        arch_get_xen_caps(&u.info);
>> +        str = u.info;
>> +        break;
>> +
>> +    default:
>> +        ASSERT_UNREACHABLE();
>> +        break;
>> +    }
>> +
>> +    if ( !str ||
>> +         !(sz = strlen(str)) )
>> +        return -ENODATA; /* failsafe */
> Is this really appropriate for e.g. an empty command line?

Hmm.  Good point.  All can in principle be empty.

In which case I think I'll put the -ENODATA in the default case and have
a return 0 for the sz == 0 case.

>> +    if ( sz > INT32_MAX )
>> +        return -E2BIG; /* Compat guests.  2G ought to be plenty. */
> While the comment here and in the public header mention compat guests,
> the check is uniform. What's the deal?

Well its either this, or a (comat ? INT32_MAX : INT64_MAX) check, along
with the ifdefary and predicates required to make that compile.

But there's not a CPU today which can actually move 2G of data (which is
4G of L1d bandwidth) without suffering the watchdog (especially as we've
just read it once for strlen(), so that's 6G of bandwidth), nor do I
expect this to change in the forseeable future.

There's some boundary (probably far lower) beyond which we can't use the
algorithm here.

There wants to be some limit, and I don't feel it is necessary to make
it variable on the guest type.

>
>> +    if ( guest_handle_is_null(arg) ) /* Length request */
>> +        return sz;
>> +
>> +    if ( copy_from_guest(&user_str, arg, 1) )
>> +        return -EFAULT;
>> +
>> +    if ( user_str.len == 0 )
>> +        return -EINVAL;
>> +
>> +    if ( sz > user_str.len )
>> +        return -ENOBUFS;
>> +
>> +    if ( copy_to_guest_offset(arg, offsetof(struct xen_var_string, buf),
>> +                              str, sz) )
>> +        return -EFAULT;
> Not inserting a nul terminator is going to make this slightly awkward to
> use.

Not really.  It matches how build-id currently works, and forces the
caller to use proper tained-string discipline.

To safely printk() it, all you need is:

rc = xen_version(XENVER_$X, &str);
if ( rc < 0 )
    return rc;
printk("%*.s\n", rc, str.buf);

>> @@ -103,6 +126,35 @@ struct xen_build_id {
>>  };
>>  typedef struct xen_build_id xen_build_id_t;
>>  
>> +/*
>> + * Container for an arbitrary variable length string.
>> + */
>> +struct xen_var_string {
>> +    uint32_t len;                          /* IN:  size of buf[] in bytes. 
>> */
>> +    unsigned char buf[XEN_FLEX_ARRAY_DIM]; /* OUT: requested data.         
>> */
>> +};
>> +typedef struct xen_var_string xen_var_string_t;
>> +
>> +/*
>> + * arg == xenver_string_t
> Nit: xen_var_string_t (also again in the following text).

Ah yes.  I originally called it xenver_string_t then found a whole load
of compat assumptions about xen prefixes on names.

Will fix.


But overall, I'm not seeing a major objection to this change?  In which
case I'll go ahead and do the tools/ cleanup too for v2.

~Andrew