Date:   Wed, 17 Nov 2021 18:37:30 -0800 (PST)
From:   Stefano Stabellini <sstabellini@kernel.org>
To:     Jan Beulich <jbeulich@suse.com>
cc:     Stefano Stabellini <sstabellini@kernel.org>,
        boris.ostrovsky@oracle.com, xen-devel@lists.xenproject.org,
        linux-kernel@vger.kernel.org,
        Stefano Stabellini <stefano.stabellini@xilinx.com>,
        stable@vger.kernel.org, jgross@suse.com
Subject: Re: [PATCH] xen: detect uninitialized xenbus in xenbus_init
In-Reply-To: <2592121c-ed62-c346-5aeb-37adb6bb1982@suse.com>
Message-ID: <alpine.DEB.2.22.394.2111171823160.1412361@ubuntu-linux-20-04-desktop>
References: <20211117021145.3105042-1-sstabellini@kernel.org> <2592121c-ed62-c346-5aeb-37adb6bb1982@suse.com>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 17 Nov 2021, Jan Beulich wrote:
> On 17.11.2021 03:11, Stefano Stabellini wrote:
> > --- a/drivers/xen/xenbus/xenbus_probe.c
> > +++ b/drivers/xen/xenbus/xenbus_probe.c
> > @@ -951,6 +951,18 @@ static int __init xenbus_init(void)
> >  		err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> >  		if (err)
> >  			goto out_error;
> > +		/*
> > +		 * Uninitialized hvm_params are zero and return no error.
> > +		 * Although it is theoretically possible to have
> > +		 * HVM_PARAM_STORE_PFN set to zero on purpose, in reality it is
> > +		 * not zero when valid. If zero, it means that Xenstore hasn't
> > +		 * been properly initialized. Instead of attempting to map a
> > +		 * wrong guest physical address return error.
> > +		 */
> > +		if (v == 0) {
> > +			err = -ENOENT;
> > +			goto out_error;
> > +		}
> 
> If such a check gets added, then I think known-invalid frame numbers
> should be covered at even higher a priority than zero.

Uhm, that's a good point. We could check for 0 and also ULONG_MAX


> This would, for example, also mean to ...
>
> >  		xen_store_gfn = (unsigned long)v;
> 
> ... stop silently truncating a value here.

Yeah, it can only happen on 32-bit but you have a point.


> By covering them we would then have the option to pre-fill PFN params
> with, say, ~0 in the hypervisor (to clearly identify them as invalid,
> rather than having to guess at the validity of 0). I haven't really
> checked yet whether such a change would be compatible with existing
> software ...

I had the same idea. I think the hvm_params should be initialized to an
invalid value in Xen. But here in Linux we need to be able to cope with
older Xen versions too so it still makes sense to check for zero in
places where it is very obviously incorrect (HVM_PARAM_STORE_PFN).


What do you think of the appended?


diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 94405bb3829e..04558d3a5562 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -951,6 +951,28 @@ static int __init xenbus_init(void)
 		err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
 		if (err)
 			goto out_error;
+		/*
+		 * Return error on an invalid value.
+		 *
+		 * Uninitialized hvm_params are zero and return no error.
+		 * Although it is theoretically possible to have
+		 * HVM_PARAM_STORE_PFN set to zero on purpose, in reality it is
+		 * not zero when valid. If zero, it means that Xenstore hasn't
+		 * been properly initialized. Instead of attempting to map a
+		 * wrong guest physical address return error.
+		 */
+		if (v == 0) {
+			err = -ENOENT;
+			goto out_error;
+		}
+		/*
+		 * ULONG_MAX is invalid on 64-bit because is INVALID_PFN.
+		 * On 32-bit return error to avoid truncation.
+		 */
+		if (v >= ULONG_MAX) {
+			err = -EINVAL;
+			goto out_error;
+		}
 		xen_store_gfn = (unsigned long)v;
 		xen_store_interface =
 			xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,