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).
> 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.
> @@ -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?
> + 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?
> + 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.
> @@ -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).
> + * Equivalent to the original ops, but with a non-truncating API/ABI.
> + *
> + * Passing arg == NULL is a request for size. The returned size does not
> + * include a NUL terminator, and has a practical upper limit of INT32_MAX for
> + * 32bit guests. This is expected to be plenty for the purpose.
As said above, the limit applies to all guests, which the wording here
doesn't suggest.
Jan
> + * Otherwise, the input xenver_string_t provides the size of the following
> + * buffer. Xen will fill the buffer, and return the number of bytes written
> + * (e.g. if the input buffer was longer than necessary).
> + *
> + * These hypercalls can fail, in which case they'll return -XEN_Exx.
> + */
> +#define XENVER_extraversion2 11
> +#define XENVER_capabilities2 12
> +#define XENVER_changeset2 13
> +#define XENVER_commandline2 14
> +
> #endif /* __XEN_PUBLIC_VERSION_H__ */
>
> /*
