linux-kernel - Re: [PATCH] drivers/xen: Improve the late XenStore init protocol

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: <alpine.DEB.2.22.394.2405151524270.2544314@ubuntu-linux-20-04-desktop>
Date: Wed, 15 May 2024 15:30:34 -0700 (PDT)
From: Stefano Stabellini <sstabellini@...nel.org>
To: Henry Wang <xin.wang2@....com>
cc: xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org, 
    Juergen Gross <jgross@...e.com>, 
    Stefano Stabellini <sstabellini@...nel.org>, 
    Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>, 
    Michal Orzel <michal.orzel@....com>
Subject: Re: [PATCH] drivers/xen: Improve the late XenStore init protocol

On Wed, 15 May 2024, Henry Wang wrote:
> Currently, the late XenStore init protocol is only triggered properly
> for the case that HVM_PARAM_STORE_PFN is ~0ULL (invalid). For the
> case that XenStore interface is allocated but not ready (the connection
> status is not XENSTORE_CONNECTED), Linux should also wait until the
> XenStore is set up properly.
> 
> Introduce a macro to describe the XenStore interface is ready, use
> it in xenbus_probe_initcall() and xenbus_probe() to select the code
> path of doing the late XenStore init protocol or not.
> 
> Take the opportunity to enhance the check of the allocated XenStore
> interface can be properly mapped, and return error early if the
> memremap() fails.
> 
> Signed-off-by: Henry Wang <xin.wang2@....com>
> Signed-off-by: Michal Orzel <michal.orzel@....com>

Please add a Fixes: tag


> ---
>  drivers/xen/xenbus/xenbus_probe.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> index 3205e5d724c8..8aec0ed1d047 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -72,6 +72,10 @@ EXPORT_SYMBOL_GPL(xen_store_evtchn);
>  struct xenstore_domain_interface *xen_store_interface;
>  EXPORT_SYMBOL_GPL(xen_store_interface);
>  
> +#define XS_INTERFACE_READY \
> +	((xen_store_interface != NULL) && \
> +	 (xen_store_interface->connection == XENSTORE_CONNECTED))
> +
>  enum xenstore_init xen_store_domain_type;
>  EXPORT_SYMBOL_GPL(xen_store_domain_type);
>  
> @@ -751,9 +755,10 @@ static void xenbus_probe(void)
>  {
>  	xenstored_ready = 1;
>  
> -	if (!xen_store_interface) {
> -		xen_store_interface = memremap(xen_store_gfn << XEN_PAGE_SHIFT,
> -					       XEN_PAGE_SIZE, MEMREMAP_WB);
> +	if (!xen_store_interface || XS_INTERFACE_READY) {
> +		if (!xen_store_interface)

These two nested if's don't make sense to me. If XS_INTERFACE_READY
succeeds, it means that  ((xen_store_interface != NULL) &&
(xen_store_interface->connection == XENSTORE_CONNECTED)).

So it is not possible that xen_store_interface == NULL immediately
after. Right?


> +			xen_store_interface = memremap(xen_store_gfn << XEN_PAGE_SHIFT,
> +						       XEN_PAGE_SIZE, MEMREMAP_WB);
>  		/*
>  		 * Now it is safe to free the IRQ used for xenstore late
>  		 * initialization. No need to unbind: it is about to be
> @@ -822,7 +827,7 @@ static int __init xenbus_probe_initcall(void)
>  	if (xen_store_domain_type == XS_PV ||
>  	    (xen_store_domain_type == XS_HVM &&
>  	     !xs_hvm_defer_init_for_callback() &&
> -	     xen_store_interface != NULL))
> +	     XS_INTERFACE_READY))
>  		xenbus_probe();
>  
>  	/*
> @@ -831,7 +836,7 @@ static int __init xenbus_probe_initcall(void)
>  	 * started, then probe.  It will be triggered when communication
>  	 * starts happening, by waiting on xb_waitq.
>  	 */
> -	if (xen_store_domain_type == XS_LOCAL || xen_store_interface == NULL) {
> +	if (xen_store_domain_type == XS_LOCAL || !XS_INTERFACE_READY) {
>  		struct task_struct *probe_task;
>  
>  		probe_task = kthread_run(xenbus_probe_thread, NULL,
> @@ -1014,6 +1019,12 @@ static int __init xenbus_init(void)
>  			xen_store_interface =
>  				memremap(xen_store_gfn << XEN_PAGE_SHIFT,
>  					 XEN_PAGE_SIZE, MEMREMAP_WB);
> +			if (!xen_store_interface) {
> +				pr_err("%s: cannot map HVM_PARAM_STORE_PFN=%llx\n",
> +				       __func__, v);
> +				err = -ENOMEM;

I think this should -EINVAL


> +				goto out_error;
> +			}
>  			if (xen_store_interface->connection != XENSTORE_CONNECTED)
>  				wait = true;
>  		}
> -- 
> 2.34.1
>