xen/memory: Make resource_max_frames() to return 0 on unknown type

Commit Message

Oleksandr Tyshchenko Feb. 16, 2025, 9:19 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

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

Fixes: 9244528955de ("xen/memory: Fix acquire_resource size semantics")
Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
---
I am not aware of any real issues with that. I just spotted that when looking
into the code. Also, I don't think there is a similar issue with acquiring
resource of an unknown type.

Another possible more invasive solution could be to make resource_max_frames()
return int (+ clarify a comment on top of it) and teach the caller to also deal with -errno
returned on error (in addition to 0). This way we can propagate an exact error (-EOPNOTSUPP)
to the VM on an unknown type. The cons are that we limit max_frames, but it seems
to me that nr_frames is limited even harder anyway down the code to fit into high-order
bits of the cmd parameter to be able to properly encode a continuation.
---
---
 xen/common/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jan Beulich Feb. 17, 2025, 9:18 a.m. UTC | #1

On 16.02.2025 22:19, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> 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).
> 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.
> 
> Fixes: 9244528955de ("xen/memory: Fix acquire_resource size semantics")
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>

Reviewed-by: Jan Beulich <[e-mail]>
albeit preferably with an addition:

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1157,7 +1157,7 @@ static unsigned int resource_max_frames(const struct domain *d,
>          return d->vmtrace_size >> PAGE_SHIFT;
>  
>      default:
> -        return -EOPNOTSUPP;
> +        return 0;
>      }
>  }

Wouldn't we better accompany this by an ASSERT_UNREACHABLE() in the default
case of _acquire_resource()?

Jan

Oleksandr Tyshchenko Feb. 17, 2025, 10:08 a.m. UTC | #2

On 17.02.25 11:18, Jan Beulich wrote:


Hello Jan

> On 16.02.2025 22:19, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <[e-mail]>
>>
>> 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).
>> 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.
>>
>> Fixes: 9244528955de ("xen/memory: Fix acquire_resource size semantics")
>> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> 
> Reviewed-by: Jan Beulich <[e-mail]>


Thanks.

> albeit preferably with an addition:
> 
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1157,7 +1157,7 @@ static unsigned int resource_max_frames(const struct domain *d,
>>           return d->vmtrace_size >> PAGE_SHIFT;
>>   
>>       default:
>> -        return -EOPNOTSUPP;
>> +        return 0;
>>       }
>>   }
> 
> Wouldn't we better accompany this by an ASSERT_UNREACHABLE() in the default
> case of _acquire_resource()?


Maybe yes, as I understand, normally we won't get to this point, as an 
unknown type will always be rejected earlier in resource_max_frames().
Will add.



> 
> Jan

Patch

diff --git a/xen/common/memory.c b/xen/common/memory.c
index a6f2f6d1b3..6ec471237b 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1157,7 +1157,7 @@  static unsigned int resource_max_frames(const struct domain *d,
         return d->vmtrace_size >> PAGE_SHIFT;
 
     default:
-        return -EOPNOTSUPP;
+        return 0;
     }
 }