[PATCH v2] xen: detect uninitialized xenbus in xenbus_init

[PATCH v2] xen: detect uninitialized xenbus in xenbus_init

[Stefano Stabellini]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

If the xenstore page hasn't been allocated properly, reading the value
of the related hvm_param (HVM_PARAM_STORE_PFN) won't actually return
error. Instead, it will succeed and return zero. Instead of attempting
to xen_remap a bad guest physical address, detect this condition and
return early.

Note that although a guest physical address of zero for
HVM_PARAM_STORE_PFN is theoretically possible, it is not a good choice
and zero has never been validly used in that capacity.

Also recognize the invalid value of INVALID_PFN which is ULLONG_MAX.

For 32-bit Linux, any pfn above ULONG_MAX would get truncated. Pfns
above ULONG_MAX should never be passed by the Xen tools to HVM guests
anyway, so check for this condition and return early.

Cc: stable@vger.kernel.org
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v2:
- add check for ULLONG_MAX (unitialized)
- add check for ULONG_MAX #if BITS_PER_LONG == 32 (actual error)
- add pr_err error message

 drivers/xen/xenbus/xenbus_probe.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 94405bb3829e..c7472ff27a93 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -951,6 +951,20 @@ static int __init xenbus_init(void)
 		err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
 		if (err)
 			goto out_error;
+		/* Uninitialized. */
+		if (v == 0 || v == ULLONG_MAX) {
+			err = -ENOENT;
+			goto out_error;
+		}
+		/* Avoid truncation on 32-bit. */
+#if BITS_PER_LONG == 32
+		if (v > ULONG_MAX) {
+			pr_err("%s: cannot handle HVM_PARAM_STORE_PFN=%llx > ULONG_MAX\n",
+			       __func__, v);
+			err = -EINVAL;
+			goto out_error;
+		}
+#endif
 		xen_store_gfn = (unsigned long)v;
 		xen_store_interface =
 			xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
-- 
2.25.1

Re: [PATCH v2] xen: detect uninitialized xenbus in xenbus_init

[Boris Ostrovsky]

On 11/19/21 3:29 PM, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> If the xenstore page hasn't been allocated properly, reading the value
> of the related hvm_param (HVM_PARAM_STORE_PFN) won't actually return
> error. Instead, it will succeed and return zero. Instead of attempting
> to xen_remap a bad guest physical address, detect this condition and
> return early.
>
> Note that although a guest physical address of zero for
> HVM_PARAM_STORE_PFN is theoretically possible, it is not a good choice
> and zero has never been validly used in that capacity.
>
> Also recognize the invalid value of INVALID_PFN which is ULLONG_MAX.
>
> For 32-bit Linux, any pfn above ULONG_MAX would get truncated. Pfns
> above ULONG_MAX should never be passed by the Xen tools to HVM guests
> anyway, so check for this condition and return early.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>


Reviewed-by: Boris Ostrovsky <boris.ostrvsky@oracle.com>

Re: [PATCH v2] xen: detect uninitialized xenbus in xenbus_init

[Jan Beulich]

On 19.11.2021 21:29, Stefano Stabellini wrote:
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -951,6 +951,20 @@ static int __init xenbus_init(void)
>  		err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
>  		if (err)
>  			goto out_error;
> +		/* Uninitialized. */
> +		if (v == 0 || v == ULLONG_MAX) {

Didn't you have a comment in v1 here regarding the check against 0? Or was that
just like now only in the description? IOW I think there ought to be a code
comment justifying the theoretically wrong check ...

Also, while I realize there are various other similar assumptions elsewhere, I
would generally recommend to avoid such: There's no guarantee that now and
forever unsigned long long and uint64_t are the same thing. And it's easy in
cases like this one:

		if (!v || !(v + 1)) {

Jan

Re: [PATCH v2] xen: detect uninitialized xenbus in xenbus_init

[Stefano Stabellini]

On Mon, 22 Nov 2021, Jan Beulich wrote:
> On 19.11.2021 21:29, Stefano Stabellini wrote:
> > --- a/drivers/xen/xenbus/xenbus_probe.c
> > +++ b/drivers/xen/xenbus/xenbus_probe.c
> > @@ -951,6 +951,20 @@ static int __init xenbus_init(void)
> >  		err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> >  		if (err)
> >  			goto out_error;
> > +		/* Uninitialized. */
> > +		if (v == 0 || v == ULLONG_MAX) {
> 
> Didn't you have a comment in v1 here regarding the check against 0? Or was that
> just like now only in the description? IOW I think there ought to be a code
> comment justifying the theoretically wrong check ...

Yeah, I added all the info in the commit message and shortened the
in-code comment this time. I am also happy to keep the details in the
in-code comment, e.g.:

/*
 * If the xenstore page hasn't been allocated properly, reading the
 * value of the related hvm_param (HVM_PARAM_STORE_PFN) won't actually
 * return error. Instead, it will succeed and return zero. Instead of
 * attempting to xen_remap a bad guest physical address, detect this
 * condition and return early.
 *
 * Note that although a guest physical address of zero for
 * HVM_PARAM_STORE_PFN is theoretically possible, it is not a good
 * choice and zero has never been validly used in that capacity.
 *
 * Also recognize the invalid value of INVALID_PFN which is ULLONG_MAX.
 */


> Also, while I realize there are various other similar assumptions elsewhere, I
> would generally recommend to avoid such: There's no guarantee that now and
> forever unsigned long long and uint64_t are the same thing. And it's easy in
> cases like this one:
> 
> 		if (!v || !(v + 1)) {

I am happy to use this.

Re: [PATCH v2] xen: detect uninitialized xenbus in xenbus_init

[Stefano Stabellini]

On Mon, 22 Nov 2021, Stefano Stabellini wrote:
> On Mon, 22 Nov 2021, Jan Beulich wrote:
> > On 19.11.2021 21:29, Stefano Stabellini wrote:
> > > --- a/drivers/xen/xenbus/xenbus_probe.c
> > > +++ b/drivers/xen/xenbus/xenbus_probe.c
> > > @@ -951,6 +951,20 @@ static int __init xenbus_init(void)
> > >  		err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> > >  		if (err)
> > >  			goto out_error;
> > > +		/* Uninitialized. */
> > > +		if (v == 0 || v == ULLONG_MAX) {
> > 
> > Didn't you have a comment in v1 here regarding the check against 0? Or was that
> > just like now only in the description? IOW I think there ought to be a code
> > comment justifying the theoretically wrong check ...
> 
> Yeah, I added all the info in the commit message and shortened the
> in-code comment this time. I am also happy to keep the details in the
> in-code comment, e.g.:
> 
> /*
>  * If the xenstore page hasn't been allocated properly, reading the
>  * value of the related hvm_param (HVM_PARAM_STORE_PFN) won't actually
>  * return error. Instead, it will succeed and return zero. Instead of
>  * attempting to xen_remap a bad guest physical address, detect this
>  * condition and return early.
>  *
>  * Note that although a guest physical address of zero for
>  * HVM_PARAM_STORE_PFN is theoretically possible, it is not a good
>  * choice and zero has never been validly used in that capacity.
>  *
>  * Also recognize the invalid value of INVALID_PFN which is ULLONG_MAX.
>  */

I sent a new version of the patch with the check below and slightly more
concise version of this comment.
 

> > Also, while I realize there are various other similar assumptions elsewhere, I
> > would generally recommend to avoid such: There's no guarantee that now and
> > forever unsigned long long and uint64_t are the same thing. And it's easy in
> > cases like this one:
> > 
> > 		if (!v || !(v + 1)) {
> 
> I am happy to use this.