Re: [PATCH v2 3/5] xen/version: Introduce non-truncating XENVER_* subops

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 16/01/2023 4:06 pm, Jan Beulich wrote:
> On 14.01.2023 00:08, Andrew Cooper wrote:
>> @@ -470,6 +471,59 @@ static int __init cf_check param_init(void)
>>  __initcall(param_init);
>>  #endif
>>  
>> +static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> +{
>> +    struct xen_varbuf user_str;
>> +    const char *str = NULL;
> This takes away from the compiler any chance of reporting "str" as
> uninitialized

Yes...

It is also the classic false positive pattern in GCC 4.x which is still
supported despite attempts to retire it.

>> +    if ( sz > KB(64) ) /* Arbitrary limit.  Avoid long-running operations. 
>> */
>> +        return -E2BIG;
>> +
>> +    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;
> The earlier of these last two checks makes it that one can't successfully
> call this function when the size query has returned 0.

This is actually a check that the build_id path already has.  I did
consider it somewhat dubious to special case 0 here, but it needs to
stay for the following patch to have no functional change.

>> --- a/xen/include/public/version.h
>> +++ b/xen/include/public/version.h
>> @@ -19,12 +19,20 @@
>>  /* arg == NULL; returns major:minor (16:16). */
>>  #define XENVER_version      0
>>  
>> -/* arg == xen_extraversion_t. */
>> +/*
>> + * arg == xen_extraversion_t.
>> + *
>> + * This API/ABI is broken.  Use XENVER_extraversion2 instead.
> Personally I don't like these "broken" that you're adding. These interfaces
> simply are the way they are, with certain limitations. We also won't be
> able to remove the old variants (except in the new ABI), so telling people
> to avoid them provides us about nothing.

Incorrect.

First, the breakage here isn't only truncation; it's char-signedness
with data that's not guaranteed to be ASCII text.  Yet another
demonstration of why C is an inappropriate way of defining an ABI.

Secondly, it is unreasonable for ABI errors and correction information
such as this not to be documented *somewhere*.  It should live with the
API technical reference, which happens to be exactly (and only) here.

These comments won't fix existing implementations.  What they will do is
cause anyone new implementing guests, or anyone who re-syncs the
headers, to notice and hopefully take mitigating action.

~Andrew