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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 (particularly by future changes; the way it's here is
obvious enough of course). The NULL also doesn't buy you anything, as
...

> +    size_t sz;
> +
> +    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:
> +        str = xen_cap_info;
> +        break;
> +
> +    default:
> +        ASSERT_UNREACHABLE();
> +        return -ENODATA;
> +    }
> +
> +    sz = strlen(str);

... we will still crash here in case the variable doesn't get any other
value assigned.

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

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

Jan