All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Sergey Dyasli <sergey.dyasli@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	xen-devel@lists.xen.org, Daniel De Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
Date: Fri, 10 Jan 2020 12:09:35 +0100	[thread overview]
Message-ID: <6086e6b5-167e-dd53-e179-27c5dfda6e09@suse.com> (raw)
In-Reply-To: <20200110103723.29538-1-sergey.dyasli@citrix.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

  parent reply	other threads:[~2020-01-10 11:10 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-10 10:37 Sergey Dyasli
2020-01-10 11:02 ` Andrew Cooper
2020-01-10 15:28   ` George Dunlap
2020-01-10 15:56     ` Jan Beulich
2020-01-10 16:45       ` Jürgen Groß
2020-01-10 17:00         ` George Dunlap
2020-01-11  3:55     ` Doug Goldstein
2020-01-11  9:35       ` George Dunlap
2020-01-13 11:01   ` Sergey Dyasli
2020-01-10 11:09 ` Jan Beulich [this message]
2020-01-11  4:02 ` Doug Goldstein
2020-01-11  9:02   ` George Dunlap
2020-01-12 18:26     ` Doug Goldstein
2020-01-13 12:51       ` George Dunlap
2020-01-13 13:39         ` Julien Grall
2020-01-13 14:01           ` Andrew Cooper
2020-01-13 14:07             ` George Dunlap
2020-01-13 14:28               ` Julien Grall
2020-01-13 14:40         ` Andrew Cooper
2020-01-14 10:19           ` Sergey Dyasli
2020-01-13 14:52         ` Julien Grall
2020-01-13 14:01       ` Ian Jackson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6086e6b5-167e-dd53-e179-27c5dfda6e09@suse.com \
    --to=jbeulich@suse.com \
    --cc=George.Dunlap@eu.citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=dgdegra@tycho.nsa.gov \
    --cc=ian.jackson@eu.citrix.com \
    --cc=julien@xen.org \
    --cc=konrad.wilk@oracle.com \
    --cc=sergey.dyasli@citrix.com \
    --cc=sstabellini@kernel.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xen.org \
    --subject='Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.