[PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM

Juergen Gross posted 1 patch 12 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230430144646.13624-1-jgross@suse.com
xen/common/sysctl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
[PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Juergen Gross 12 months ago
In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
can fail if the last domain scanned isn't allowed to be accessed by
the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).

Fix that by just ignoring scanned domains where xsm_getdomaininfo()
is returning an error, like it is effectively done when such a
situation occurs for a domain not being the last one scanned.

Fixes: d046f361dc93 ("Xen Security Modules: XSM")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sysctl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 02505ab044..0cbfe8bd44 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
             if ( num_domains == op->u.getdomaininfolist.max_domains )
                 break;
 
-            ret = xsm_getdomaininfo(XSM_HOOK, d);
-            if ( ret )
+            if ( xsm_getdomaininfo(XSM_HOOK, d) )
                 continue;
 
             getdomaininfo(d, &info);
-- 
2.35.3
Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Daniel P. Smith 12 months ago
On 4/30/23 10:46, Juergen Gross wrote:
> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
> can fail if the last domain scanned isn't allowed to be accessed by
> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).
> 
> Fix that by just ignoring scanned domains where xsm_getdomaininfo()
> is returning an error, like it is effectively done when such a
> situation occurs for a domain not being the last one scanned.
> 
> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   xen/common/sysctl.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 02505ab044..0cbfe8bd44 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>               if ( num_domains == op->u.getdomaininfolist.max_domains )
>                   break;
>   
> -            ret = xsm_getdomaininfo(XSM_HOOK, d);
> -            if ( ret )
> +            if ( xsm_getdomaininfo(XSM_HOOK, d) )
>                   continue;
>   
>               getdomaininfo(d, &info);


This change does not match the commit message. This says it fixes an 
issue, but unless I am totally missing something, this change is nothing 
more than formatting that drops the use of an intermediate variable. 
Please feel free to correct me if I am wrong here, otherwise I believe 
the commit message should be changed to reflect the code change.

Second, as far as the problem description goes. The *only* time the call 
to xsm_getdomaininfo() at this location will return anything other than 
0, is when FLASK is being used and a domain whose type is not allowed 
getdomaininfo is making the call. XSM_HOOK signals a no-op check for the 
default/dummy policy, and the SILO policy does not override the 
default/dummy policy for this check.

V/r,
Daniel P. Smith
Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Juergen Gross 12 months ago
On 02.05.23 15:03, Daniel P. Smith wrote:
> On 4/30/23 10:46, Juergen Gross wrote:
>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
>> can fail if the last domain scanned isn't allowed to be accessed by
>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).
>>
>> Fix that by just ignoring scanned domains where xsm_getdomaininfo()
>> is returning an error, like it is effectively done when such a
>> situation occurs for a domain not being the last one scanned.
>>
>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/common/sysctl.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>> index 02505ab044..0cbfe8bd44 100644
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>               if ( num_domains == op->u.getdomaininfolist.max_domains )
>>                   break;
>> -            ret = xsm_getdomaininfo(XSM_HOOK, d);
>> -            if ( ret )
>> +            if ( xsm_getdomaininfo(XSM_HOOK, d) )
>>                   continue;
>>               getdomaininfo(d, &info);
> 
> 
> This change does not match the commit message. This says it fixes an issue, but 
> unless I am totally missing something, this change is nothing more than 
> formatting that drops the use of an intermediate variable. Please feel free to 
> correct me if I am wrong here, otherwise I believe the commit message should be 
> changed to reflect the code change.

You are missing the fact that ret getting set by a failing xsm_getdomaininfo()
call might result in the ret value being propagated to the sysctl caller. And
this should not happen. So the fix is to NOT modify ret here.

> Second, as far as the problem description goes. The *only* time the call to 
> xsm_getdomaininfo() at this location will return anything other than 0, is when 
> FLASK is being used and a domain whose type is not allowed getdomaininfo is 
> making the call. XSM_HOOK signals a no-op check for the default/dummy policy, 
> and the SILO policy does not override the default/dummy policy for this check.

Your statement sounds as if xsm_getdomaininfo() would always return the same
value for a given caller domain. Isn't that return value also depending on the
domain specified via the second parameter? In case it isn't, why does that
parameter even exist?


Juergen
Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Daniel P. Smith 12 months ago
On 5/2/23 09:13, Juergen Gross wrote:
> On 02.05.23 15:03, Daniel P. Smith wrote:
>> On 4/30/23 10:46, Juergen Gross wrote:
>>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
>>> can fail if the last domain scanned isn't allowed to be accessed by
>>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).
>>>
>>> Fix that by just ignoring scanned domains where xsm_getdomaininfo()
>>> is returning an error, like it is effectively done when such a
>>> situation occurs for a domain not being the last one scanned.
>>>
>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>   xen/common/sysctl.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>>> index 02505ab044..0cbfe8bd44 100644
>>> --- a/xen/common/sysctl.c
>>> +++ b/xen/common/sysctl.c
>>> @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
>>> u_sysctl)
>>>               if ( num_domains == op->u.getdomaininfolist.max_domains )
>>>                   break;
>>> -            ret = xsm_getdomaininfo(XSM_HOOK, d);
>>> -            if ( ret )
>>> +            if ( xsm_getdomaininfo(XSM_HOOK, d) )
>>>                   continue;
>>>               getdomaininfo(d, &info);
>>
>>
>> This change does not match the commit message. This says it fixes an 
>> issue, but unless I am totally missing something, this change is 
>> nothing more than formatting that drops the use of an intermediate 
>> variable. Please feel free to correct me if I am wrong here, otherwise 
>> I believe the commit message should be changed to reflect the code 
>> change.
> 
> You are missing the fact that ret getting set by a failing 
> xsm_getdomaininfo()
> call might result in the ret value being propagated to the sysctl 
> caller. And
> this should not happen. So the fix is to NOT modify ret here.

You are correct, my apologies for that.

>> Second, as far as the problem description goes. The *only* time the 
>> call to xsm_getdomaininfo() at this location will return anything 
>> other than 0, is when FLASK is being used and a domain whose type is 
>> not allowed getdomaininfo is making the call. XSM_HOOK signals a no-op 
>> check for the default/dummy policy, and the SILO policy does not 
>> override the default/dummy policy for this check.
> 
> Your statement sounds as if xsm_getdomaininfo() would always return the 
> same
> value for a given caller domain. Isn't that return value also depending 
> on the
> domain specified via the second parameter? In case it isn't, why does that
> parameter even exist?

It would if the default action was something other than XSM_HOOK. Look 
at line 82 of include/xsm/dummy.h. XSM_HOOK will always return 0 
regardless of the src or dest domains. The function xsm_defualt_action() 
is the policy for both default/dummy and SILO with the exception for 
evntchn, grants, and argo checks for SILO.

v/r,
DPS

Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Juergen Gross 12 months ago
On 02.05.23 15:23, Daniel P. Smith wrote:
> On 5/2/23 09:13, Juergen Gross wrote:
>> On 02.05.23 15:03, Daniel P. Smith wrote:
>>> On 4/30/23 10:46, Juergen Gross wrote:
>>>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
>>>> can fail if the last domain scanned isn't allowed to be accessed by
>>>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).
>>>>
>>>> Fix that by just ignoring scanned domains where xsm_getdomaininfo()
>>>> is returning an error, like it is effectively done when such a
>>>> situation occurs for a domain not being the last one scanned.
>>>>
>>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>   xen/common/sysctl.c | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>>>> index 02505ab044..0cbfe8bd44 100644
>>>> --- a/xen/common/sysctl.c
>>>> +++ b/xen/common/sysctl.c
>>>> @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>>>               if ( num_domains == op->u.getdomaininfolist.max_domains )
>>>>                   break;
>>>> -            ret = xsm_getdomaininfo(XSM_HOOK, d);
>>>> -            if ( ret )
>>>> +            if ( xsm_getdomaininfo(XSM_HOOK, d) )
>>>>                   continue;
>>>>               getdomaininfo(d, &info);
>>>
>>>
>>> This change does not match the commit message. This says it fixes an issue, 
>>> but unless I am totally missing something, this change is nothing more than 
>>> formatting that drops the use of an intermediate variable. Please feel free 
>>> to correct me if I am wrong here, otherwise I believe the commit message 
>>> should be changed to reflect the code change.
>>
>> You are missing the fact that ret getting set by a failing xsm_getdomaininfo()
>> call might result in the ret value being propagated to the sysctl caller. And
>> this should not happen. So the fix is to NOT modify ret here.
> 
> You are correct, my apologies for that.

No need to apologize. :-)

>>> Second, as far as the problem description goes. The *only* time the call to 
>>> xsm_getdomaininfo() at this location will return anything other than 0, is 
>>> when FLASK is being used and a domain whose type is not allowed getdomaininfo 
>>> is making the call. XSM_HOOK signals a no-op check for the default/dummy 
>>> policy, and the SILO policy does not override the default/dummy policy for 
>>> this check.
>>
>> Your statement sounds as if xsm_getdomaininfo() would always return the same
>> value for a given caller domain. Isn't that return value also depending on the
>> domain specified via the second parameter? In case it isn't, why does that
>> parameter even exist?
> 
> It would if the default action was something other than XSM_HOOK. Look at line 
> 82 of include/xsm/dummy.h. XSM_HOOK will always return 0 regardless of the src 
> or dest domains. The function xsm_defualt_action() is the policy for both 
> default/dummy and SILO with the exception for evntchn, grants, and argo checks 
> for SILO.

Ah, okay. I didn't analyze all of the involved xsm code.


Juergen
Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Daniel P. Smith 12 months ago
On 5/2/23 09:30, Juergen Gross wrote:
> On 02.05.23 15:23, Daniel P. Smith wrote:
>> On 5/2/23 09:13, Juergen Gross wrote:
>>> On 02.05.23 15:03, Daniel P. Smith wrote:
>>>> On 4/30/23 10:46, Juergen Gross wrote:
>>>>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
>>>>> can fail if the last domain scanned isn't allowed to be accessed by
>>>>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).
>>>>>
>>>>> Fix that by just ignoring scanned domains where xsm_getdomaininfo()
>>>>> is returning an error, like it is effectively done when such a
>>>>> situation occurs for a domain not being the last one scanned.
>>>>>
>>>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>>   xen/common/sysctl.c | 3 +--
>>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>>>>> index 02505ab044..0cbfe8bd44 100644
>>>>> --- a/xen/common/sysctl.c
>>>>> +++ b/xen/common/sysctl.c
>>>>> @@ -89,8 +89,7 @@ long 
>>>>> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>>>>               if ( num_domains == 
>>>>> op->u.getdomaininfolist.max_domains )
>>>>>                   break;
>>>>> -            ret = xsm_getdomaininfo(XSM_HOOK, d);
>>>>> -            if ( ret )
>>>>> +            if ( xsm_getdomaininfo(XSM_HOOK, d) )
>>>>>                   continue;
>>>>>               getdomaininfo(d, &info);
>>>>
>>>>
>>>> This change does not match the commit message. This says it fixes an 
>>>> issue, but unless I am totally missing something, this change is 
>>>> nothing more than formatting that drops the use of an intermediate 
>>>> variable. Please feel free to correct me if I am wrong here, 
>>>> otherwise I believe the commit message should be changed to reflect 
>>>> the code change.
>>>
>>> You are missing the fact that ret getting set by a failing 
>>> xsm_getdomaininfo()
>>> call might result in the ret value being propagated to the sysctl 
>>> caller. And
>>> this should not happen. So the fix is to NOT modify ret here.
>>
>> You are correct, my apologies for that.
> 
> No need to apologize. :-)

I believe it is proper to admit when you are wrong.

>>>> Second, as far as the problem description goes. The *only* time the 
>>>> call to xsm_getdomaininfo() at this location will return anything 
>>>> other than 0, is when FLASK is being used and a domain whose type is 
>>>> not allowed getdomaininfo is making the call. XSM_HOOK signals a 
>>>> no-op check for the default/dummy policy, and the SILO policy does 
>>>> not override the default/dummy policy for this check.
>>>
>>> Your statement sounds as if xsm_getdomaininfo() would always return 
>>> the same
>>> value for a given caller domain. Isn't that return value also 
>>> depending on the
>>> domain specified via the second parameter? In case it isn't, why does 
>>> that
>>> parameter even exist?
>>
>> It would if the default action was something other than XSM_HOOK. Look 
>> at line 82 of include/xsm/dummy.h. XSM_HOOK will always return 0 
>> regardless of the src or dest domains. The function 
>> xsm_defualt_action() is the policy for both default/dummy and SILO 
>> with the exception for evntchn, grants, and argo checks for SILO.
> 
> Ah, okay. I didn't analyze all of the involved xsm code.

No worries! I am always willing to help in any way that I can. While I 
don't have the bandwidth to be proactive and keep up with everything on 
xen-devel, please do not hesitate to ask me or ping me on anything XSM 
related. I will gladly take a look and provide what insights I might 
have on your query.

v/r,
dps

Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Daniel P. Smith 12 months ago
On 5/2/23 09:23, Daniel P. Smith wrote:
> On 5/2/23 09:13, Juergen Gross wrote:
>> On 02.05.23 15:03, Daniel P. Smith wrote:
>>> On 4/30/23 10:46, Juergen Gross wrote:
>>>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
>>>> can fail if the last domain scanned isn't allowed to be accessed by
>>>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).
>>>>
>>>> Fix that by just ignoring scanned domains where xsm_getdomaininfo()
>>>> is returning an error, like it is effectively done when such a
>>>> situation occurs for a domain not being the last one scanned.
>>>>
>>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>   xen/common/sysctl.c | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>>>> index 02505ab044..0cbfe8bd44 100644
>>>> --- a/xen/common/sysctl.c
>>>> +++ b/xen/common/sysctl.c
>>>> @@ -89,8 +89,7 @@ long 
>>>> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>>>               if ( num_domains == op->u.getdomaininfolist.max_domains )
>>>>                   break;
>>>> -            ret = xsm_getdomaininfo(XSM_HOOK, d);
>>>> -            if ( ret )
>>>> +            if ( xsm_getdomaininfo(XSM_HOOK, d) )
>>>>                   continue;
>>>>               getdomaininfo(d, &info);
>>>
>>>
>>> This change does not match the commit message. This says it fixes an 
>>> issue, but unless I am totally missing something, this change is 
>>> nothing more than formatting that drops the use of an intermediate 
>>> variable. Please feel free to correct me if I am wrong here, 
>>> otherwise I believe the commit message should be changed to reflect 
>>> the code change.
>>
>> You are missing the fact that ret getting set by a failing 
>> xsm_getdomaininfo()
>> call might result in the ret value being propagated to the sysctl 
>> caller. And
>> this should not happen. So the fix is to NOT modify ret here.
> 
> You are correct, my apologies for that.
> 
>>> Second, as far as the problem description goes. The *only* time the 
>>> call to xsm_getdomaininfo() at this location will return anything 
>>> other than 0, is when FLASK is being used and a domain whose type is 
>>> not allowed getdomaininfo is making the call. XSM_HOOK signals a 
>>> no-op check for the default/dummy policy, and the SILO policy does 
>>> not override the default/dummy policy for this check.
>>
>> Your statement sounds as if xsm_getdomaininfo() would always return 
>> the same
>> value for a given caller domain. Isn't that return value also 
>> depending on the
>> domain specified via the second parameter? In case it isn't, why does 
>> that
>> parameter even exist?
> 
> It would if the default action was something other than XSM_HOOK. Look 
> at line 82 of include/xsm/dummy.h. XSM_HOOK will always return 0 
> regardless of the src or dest domains. The function xsm_defualt_action() 
> is the policy for both default/dummy and SILO with the exception for 
> evntchn, grants, and argo checks for SILO.

Sorry, one last clarification. xsm_default_action() is also what is used 
when XSM=n. The difference is that for XSM=n, xsm_default_action() is 
in-lined at the call site whereas with XSM=y and not using FLASK results 
in a function call xsm_default_action().

v/r,
dps

Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Roger Pau Monné 12 months ago
On Tue, May 02, 2023 at 09:03:00AM -0400, Daniel P. Smith wrote:
> On 4/30/23 10:46, Juergen Gross wrote:
> > In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
> > can fail if the last domain scanned isn't allowed to be accessed by
> > the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).
> > 
> > Fix that by just ignoring scanned domains where xsm_getdomaininfo()
> > is returning an error, like it is effectively done when such a
> > situation occurs for a domain not being the last one scanned.
> > 
> > Fixes: d046f361dc93 ("Xen Security Modules: XSM")
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> > ---
> >   xen/common/sysctl.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> > index 02505ab044..0cbfe8bd44 100644
> > --- a/xen/common/sysctl.c
> > +++ b/xen/common/sysctl.c
> > @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> >               if ( num_domains == op->u.getdomaininfolist.max_domains )
> >                   break;
> > -            ret = xsm_getdomaininfo(XSM_HOOK, d);
> > -            if ( ret )
> > +            if ( xsm_getdomaininfo(XSM_HOOK, d) )
> >                   continue;
> >               getdomaininfo(d, &info);
> 
> 
> This change does not match the commit message. This says it fixes an issue,
> but unless I am totally missing something, this change is nothing more than
> formatting that drops the use of an intermediate variable. Please feel free
> to correct me if I am wrong here, otherwise I believe the commit message
> should be changed to reflect the code change.

By dropping that intermediate variable it prevents returning an error
as the result of the hypercall if xsm_getdomaininfo() for the last
domain fails.

Note that xsm_getdomaininfo() failing for other domains not the last
one don't cause the return value of the hypercall to be an error
code, because the variable containing the error gets overwritten by
further loops.

Regards, Roger.
Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Daniel P. Smith 12 months ago
On 5/2/23 09:10, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 09:03:00AM -0400, Daniel P. Smith wrote:
>> On 4/30/23 10:46, Juergen Gross wrote:
>>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
>>> can fail if the last domain scanned isn't allowed to be accessed by
>>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).
>>>
>>> Fix that by just ignoring scanned domains where xsm_getdomaininfo()
>>> is returning an error, like it is effectively done when such a
>>> situation occurs for a domain not being the last one scanned.
>>>
>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>    xen/common/sysctl.c | 3 +--
>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>>> index 02505ab044..0cbfe8bd44 100644
>>> --- a/xen/common/sysctl.c
>>> +++ b/xen/common/sysctl.c
>>> @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>>                if ( num_domains == op->u.getdomaininfolist.max_domains )
>>>                    break;
>>> -            ret = xsm_getdomaininfo(XSM_HOOK, d);
>>> -            if ( ret )
>>> +            if ( xsm_getdomaininfo(XSM_HOOK, d) )
>>>                    continue;
>>>                getdomaininfo(d, &info);
>>
>>
>> This change does not match the commit message. This says it fixes an issue,
>> but unless I am totally missing something, this change is nothing more than
>> formatting that drops the use of an intermediate variable. Please feel free
>> to correct me if I am wrong here, otherwise I believe the commit message
>> should be changed to reflect the code change.
> 
> By dropping that intermediate variable it prevents returning an error
> as the result of the hypercall if xsm_getdomaininfo() for the last
> domain fails.

Ah, understood. I missed ret is state tracking.

> Note that xsm_getdomaininfo() failing for other domains not the last
> one don't cause the return value of the hypercall to be an error
> code, because the variable containing the error gets overwritten by
> further loops.

In the end, this is just addressing an issue that has not been seen by 
anyone and happened upon while debugging another issue.

V/r,
DPS

Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
Posted by Jan Beulich 12 months ago
On 30.04.2023 16:46, Juergen Gross wrote:
> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist
> can fail if the last domain scanned isn't allowed to be accessed by
> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing).
> 
> Fix that by just ignoring scanned domains where xsm_getdomaininfo()
> is returning an error, like it is effectively done when such a
> situation occurs for a domain not being the last one scanned.
> 
> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Should have finished reading the list first, before sending the same
patch again ...

Jan