Whether a domain is allowed to issue cache-control operations is reported
by the cache_flush_permitted() check.  Introduce such check to limit the
availability of GNTTABOP_cache_flush to only guests that are granted cache
control.

Fixes: 18e8d22fe750 ("introduce GNTTABOP_cache_flush")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/grant_table.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index e75ff98aff1c..d874ac5f1241 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3780,6 +3780,11 @@ long do_grant_table_op(
 
         if ( unlikely(!guest_handle_okay(cflush, count)) )
             goto out;
+
+        rc = -EPERM;
+        if ( !cache_flush_permitted(current->domain) )
+            goto out;
+
         rc = gnttab_cache_flush(cflush, &opaque_in, count);
         if ( rc >= 0 )
         {
-- 
2.48.1

Hi Roger,

On 06/05/2025 09:31, Roger Pau Monne wrote:
> Whether a domain is allowed to issue cache-control operations is reported
> by the cache_flush_permitted() check.  Introduce such check to limit the
> availability of GNTTABOP_cache_flush to only guests that are granted cache
> control.

Can you outline what's the problem you are trying to solve? Asking, 
because I don't see the problem of allowing any guest calling 
GNTTABOP_cache_flush on Arm from any domains.

Cheers,

-- 
Julien Grall

On Tue, May 06, 2025 at 11:15:09AM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 06/05/2025 09:31, Roger Pau Monne wrote:
> > Whether a domain is allowed to issue cache-control operations is reported
> > by the cache_flush_permitted() check.  Introduce such check to limit the
> > availability of GNTTABOP_cache_flush to only guests that are granted cache
> > control.
> 
> Can you outline what's the problem you are trying to solve? Asking, because
> I don't see the problem of allowing any guest calling GNTTABOP_cache_flush
> on Arm from any domains.

At least on x86 cache flush operations are restricted to guests for
which cache_flush_permitted() returns true.  I've assumed the same
would apply to Arm, since cache_flush_permitted() is also defined
there.  If it's fine to issue cache flush operations from any guests
on ARM, I suggest cache_flush_permitted() should unconditionally
return true then.

The problem on x86 is that it's an expensive operation when done
correctly, as it involves flushing the caches of all pCPUs where the
vCPU has been scheduled.  Note however the implementation of
GNTTABOP_cache_flush is incorrect on x86, and won't work as
expected.

Thanks, Roger.

On 06.05.2025 12:40, Roger Pau Monné wrote:
> On Tue, May 06, 2025 at 11:15:09AM +0100, Julien Grall wrote:
>> On 06/05/2025 09:31, Roger Pau Monne wrote:
>>> Whether a domain is allowed to issue cache-control operations is reported
>>> by the cache_flush_permitted() check.  Introduce such check to limit the
>>> availability of GNTTABOP_cache_flush to only guests that are granted cache
>>> control.
>>
>> Can you outline what's the problem you are trying to solve? Asking, because
>> I don't see the problem of allowing any guest calling GNTTABOP_cache_flush
>> on Arm from any domains.
> 
> At least on x86 cache flush operations are restricted to guests for
> which cache_flush_permitted() returns true.  I've assumed the same
> would apply to Arm, since cache_flush_permitted() is also defined
> there.  If it's fine to issue cache flush operations from any guests
> on ARM, I suggest cache_flush_permitted() should unconditionally
> return true then.
> 
> The problem on x86 is that it's an expensive operation when done
> correctly, as it involves flushing the caches of all pCPUs where the
> vCPU has been scheduled.  Note however the implementation of
> GNTTABOP_cache_flush is incorrect on x86, and won't work as
> expected.

So instead of altering Arm behavior, how about rejecting GNTTABOP_cache_flush
on x86 then? It was introduced specifically for Arm, and it shouldn't have
gained any users (albeit of course we can't be sure of that).

Jan