Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
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
---
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, );
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
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
---
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, );
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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
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
---
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, );
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
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
---
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, );
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
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
---
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, );
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
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
---
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, );
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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM
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
> > ---
> > 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, );
>
>
> 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
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
---
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, );
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
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
Reviewed-by: Jan Beulich
Should have finished reading the list first, before sending the same
patch again ...
Jan
