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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/02/2025 1:11 pm, Oleksandr Tyshchenko wrote:
>
>
> 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.
>

https://lore.kernel.org/xen-devel/cover.36ee649a8537af1a5fb5b3c5b7ffc0d8a1369969.1739496480.git-series.marmarek@xxxxxxxxxxxxxxxxxxxxxx


wires these tests up in Gitlab CI.

>
>
> 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

s/for //, I think?

> +     * of uint16_t type.
> +     */
> +    rc = xenforeignmemory_resource_size(
> +        fh, domid, 65535,
> +        XENMEM_resource_grant_table_id_shared, &size);

XENMEM_resource_grant_table_id_shared should probably be 0 here.

But, I'd suggest choosing 3 (literal 3, not some kind of constant from
the headers) for the major resource number.  That has the side effect of
forcing people to extend this test case when they add a new resource type.

> +
> +    /*
> +     * 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");

I'd phrase this differently.  "Check that Xen rejected the resource type."

"avoid this bug we already fixed" won't be useful to people reading this
code in the future.  It's in the commit message.

~Andrew