Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests

[Jan Beulich <jbeulich@suse.com>]

On 10.01.2020 11:37, Sergey Dyasli wrote:
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -995,6 +995,12 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
>      hvm_param_set(HVM_PARAM_VM_GENERATION_ID_ADDR, config->vm_gid_addr);
>  }
>  
> +void xsm_filter_denied(char *str, size_t len)
> +{
> +    if ( strcmp(str, "<denied>") == 0 )
> +        memset(str, 0, len);
> +}

I think you can get away without passing in "len":

void xsm_filter_denied(char *str, size_t len)
{
    if ( strcmp(str, "<denied>") == 0 )
        *str = 0;
}

Any reason you think you need to zap the entire buffer?

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -750,14 +750,17 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op)
>      case XENVER_get_features:
>          /* These sub-ops ignore the permission checks and return data. */
>          return 0;
> -    case XENVER_extraversion:

Could you take the opportunity and also add the missing blank
line here, just like you do below?

> -    case XENVER_compile_info:
>      case XENVER_capabilities:
> -    case XENVER_changeset:
>      case XENVER_pagesize:
>      case XENVER_guest_handle:
>          /* These MUST always be accessible to any guest by default. */
>          return xsm_default_action(XSM_HOOK, current->domain, NULL);
> +
> +    case XENVER_extraversion:
> +    case XENVER_compile_info:
> +    case XENVER_changeset:
> +    case XENVER_commandline:
> +    case XENVER_build_id:
>      default:
>          return xsm_default_action(XSM_PRIV, current->domain, NULL);

I continue to not see the need to have "default:" accompanied by
various specific case labels. I don't think we do so elsewhere.
And if you do, "default:" should gain ASSERT_UNREACHABLE() imo. I
also remain unconvinced of the very brief reasoning - as indicated
before, it would seem at least desirable to me to discuss why the
previous decision was wrong (iirc the implication back then was
that anyone wanting to tighten this would be supposed to use a
respective XSM policy).

In any event - if the hvmloader change was submitted as a separate
patch, I'd ack it with the change suggested (or a suitable verbal
clarification in reply to my remark above).

Jan

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