linux-kernel - Re: [PATCH] drivers/xen/hypervisor: Expose Xen SIF flags to userspace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <15fff7b8-b030-edf9-5bff-073b9757cb2e@suse.com>
Date:   Fri, 9 Dec 2022 10:04:41 +0100
From:   Juergen Gross <jgross@...e.com>
To:     Per Bilse <per.bilse@...rix.com>, linux-kernel@...r.kernel.org
Cc:     Stefano Stabellini <sstabellini@...nel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        "moderated list:XEN HYPERVISOR INTERFACE" 
        <xen-devel@...ts.xenproject.org>
Subject: Re: [PATCH] drivers/xen/hypervisor: Expose Xen SIF flags to userspace

On 02.12.22 19:22, Per Bilse wrote:
> /proc/xen is a legacy pseudo filesystem which predates Xen support
> getting merged into Linux.  It has largely been replaced with more
> normal locations for data (/sys/hypervisor/ for info, /dev/xen/ for
> user devices).  We want to compile xenfs support out of the dom0 kernel.
> 
> There is one item which only exists in /proc/xen, namely
> /proc/xen/capabilities with "control_d" being the signal of "you're in
> the control domain".  This ultimately comes from the SIF flags provided
> at VM start.
> 
> This patch exposes all SIF flags in /sys/hypervisor/start_flags/ as
> boolean files, one for each bit, returning '1' if set, '0' otherwise.
> Two known flags, 'privileged' and 'initdomain', are explicitly named,
> and all remaining flags can be accessed via generically named files,
> as suggested by Andrew Cooper.
> 
> This patch replaces previous suggestion for SIF flags exposure in its
> entirety.
> 
> Signed-off-by: Per Bilse <per.bilse@...rix.com>
> ---
>   Documentation/ABI/stable/sysfs-hypervisor-xen |  8 +++
>   drivers/xen/sys-hypervisor.c                  | 70 +++++++++++++++++--
>   2 files changed, 74 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/stable/sysfs-hypervisor-xen b/Documentation/ABI/stable/sysfs-hypervisor-xen
> index 748593c64568..f52f98548184 100644
> --- a/Documentation/ABI/stable/sysfs-hypervisor-xen
> +++ b/Documentation/ABI/stable/sysfs-hypervisor-xen
> @@ -120,3 +120,11 @@ Contact:	xen-devel@...ts.xenproject.org
>   Description:	If running under Xen:
>   		The Xen version is in the format <major>.<minor><extra>
>   		This is the <minor> part of it.
> +
> +What:		/sys/hypervisor/start_flags/*
> +Date:		December 2022
> +KernelVersion:	6.1.0
> +Contact:	xen-devel@...ts.xenproject.org
> +Description:	If running under Xen:
> +		All bits in Xen's start-flags are represented as
> +		boolean files, returning '1' if set, '0' otherwise.

I think at least the files which want to be used by e.g. systemd
("initdomain" as replacement for the "control_d" string in capabilities,
but I think "privileged" as well) should be explicitly added to this
description, as those are meant to be used as a stable ABI.

> diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
> index fcb0792f090e..e15d3ff2c56f 100644
> --- a/drivers/xen/sys-hypervisor.c
> +++ b/drivers/xen/sys-hypervisor.c
> @@ -31,7 +31,10 @@ struct hyp_sysfs_attr {
>   	struct attribute attr;
>   	ssize_t (*show)(struct hyp_sysfs_attr *, char *);
>   	ssize_t (*store)(struct hyp_sysfs_attr *, const char *, size_t);
> -	void *hyp_attr_data;
> +	union {
> +		void *hyp_attr_data;
> +		unsigned long hyp_attr_value;
> +	};
>   };
>   
>   static ssize_t type_show(struct hyp_sysfs_attr *attr, char *buffer)
> @@ -399,6 +402,61 @@ static int __init xen_sysfs_properties_init(void)
>   	return sysfs_create_group(hypervisor_kobj, &xen_properties_group);
>   }
>   
> +#define FLAG_UNAME "unknown"
> +#define FLAG_UNAME_FMT FLAG_UNAME "%02u"
> +#define FLAG_UNAME_MAX sizeof(FLAG_UNAME "XX")
> +#define FLAG_COUNT (sizeof(xen_start_flags) * BITS_PER_BYTE)
> +static_assert(sizeof(xen_start_flags)
> +		<= sizeof_field(struct hyp_sysfs_attr, hyp_attr_value));
> +
> +static ssize_t flag_show(struct hyp_sysfs_attr *attr, char *buffer)
> +{
> +	char *p = buffer;
> +
> +	*p++ = '0' + ((xen_start_flags & attr->hyp_attr_value) != 0);
> +	*p++ = '\n';
> +	return p - buffer;
> +}
> +
> +/*
> +* Add new, known flags here.  No other changes are required, but
> +* note that each known flag wastes one entry in flag_unames[].
> +* The code/complexity machinations to avoid this isn't worth it
> +* for a few entries, but keep it in mind.
> +*/
> +static struct hyp_sysfs_attr flag_attrs[FLAG_COUNT] = {
> +	[ilog2(SIF_PRIVILEGED)] = {
> +		.attr = { .name = "privileged", .mode = 0444 },
> +		.show = flag_show,
> +		.hyp_attr_value = SIF_PRIVILEGED
> +	},

What about:

#define FLAG_NODE(bit, node)					\
	[ilog2(bit)] = {					\
		.attr = { .name = #node, .mode = 0444 },	\
		.show = flag_show,				\
		.hyp_attr_value = bit				\
	}

FLAG_NODE(SIF_PRIVILEGED, privileged),
FLAG_NODE(SIF_INITDOMAIN, initdomain)

> +	[ilog2(SIF_INITDOMAIN)] = {
> +		.attr = { .name = "initdomain", .mode = 0444 },
> +		.show = flag_show,
> +		.hyp_attr_value = SIF_INITDOMAIN
> +	}

In order to avoid a consumer having to look into the sources for any other
set flag, maybe add names for currently defined flags, too? Or just skip
the other flags and add a single additional node ("flags"?) with the whole
value of xen_start_flags either in hex or binary form?

Please note that this is a suggestion only, I'm not insisting on it.


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)