Re: [PATCH V2] xen/memory: Make resource_max_frames() to return 0 on unknown type

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 17.02.25 13:09, Andrew Cooper wrote:


Hello Andrew


> On 17/02/2025 10:27 am, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > This is actually what the caller acquire_resource() expects on any kind
> > of error (the comment on top of resource_max_frames() also suggests that).
> :(
>
> So it broke somewhere between v3 and v8 of the original patch series,
> but I can't seem to find the intervening series on lore.
>
> Given the comment, I suspect I got inadvertently-reviewed into this bug.
>
> > Otherwise, the caller will treat -errno as a valid value and propagate incorrect
> > nr_frames to the VM. As a possible consequence, a VM trying to query a resource
> > size of an unknown type will get the success result from the hypercall and 
> > obtain
> > nr_frames 4294967201.
> This is one of the few interfaces we have low level testing for.
>
> tools/tests/resource/test-resource.c
yes

> Please could you add a step looking for an invalid resource id and check
> you get 0 back?


Sure. I was thinking where to add this step and propose the following 
change. I will send a formal patch once I find a way how to easily test 
this change.


From 872565da55b7e87e1664714bb1b3ee84245db4a5 Mon Sep 17 00:00:00 2001
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
Date: Mon, 17 Feb 2025 14:16:50 +0200
Subject: [PATCH] tests/resource: Verify that Xen can deal with invalid
 resource type

The sign of the presence of a corresponding bugfix is an error
returned on querying the size of an unknown for Xen resource type.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
---
 tools/tests/resource/test-resource.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/tests/resource/test-resource.c 
b/tools/tests/resource/test-resource.c
index 1b10be16a6..197477c3f3 100644
--- a/tools/tests/resource/test-resource.c
+++ b/tools/tests/resource/test-resource.c
@@ -123,6 +123,22 @@ static void test_gnttab(uint32_t domid, unsigned 
int nr_frames,
         fail("    Fail: Managed to map gnttab v2 status frames in v1 
mode\n");
         xenforeignmemory_unmap_resource(fh, res);
     }
+
+    /*
+     * While at it, verify that an attempt to query the size of an unknown
+     * for Xen resource type fails. Use the highest possible value for 
variable
+     * of uint16_t type.
+     */
+    rc = xenforeignmemory_resource_size(
+        fh, domid, 65535,
+        XENMEM_resource_grant_table_id_shared, &size);
+
+    /*
+     * Success here indicates that Xen is missing the bugfix to make size
+     * requests return an error on an invalid resource type.
+     */
+    if ( !rc )
+        fail("    Fail: Expected error on an invalid resource type, got 
success\n");
 }

 static void test_domain_configurations(void)
--
2.34.1




> > Also, add an ASSERT_UNREACHABLE() in the default case of _acquire_resource(),
> > normally we won't get to this point, as an unknown type will always be rejected
> > earlier in resource_max_frames().
> Fixes: 9244528955de ("xen/memory: Fix acquire_resource size semantics")
>
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ~Andrew