Re: [PATCH] sysctl: XSM hook should not cause XEN_SYSCTL_getdomaininfolist to (appear to) fail

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 5/2/23 06:59, Jan Beulich wrote:
> On 02.05.2023 12:43, Daniel P. Smith wrote:
> > On 5/2/23 03:17, Jan Beulich wrote:
> > > Unlike for XEN_DOMCTL_getdomaininfo, where the XSM check is intended to
> > > cause the operation to fail, in the loop here it ought to merely
> > > determine whether information for the domain at hand may be reported
> > > back. Therefore if on the last iteration the hook results in denial,
> > > this should not affect the sub-op's return value.
> > >
> > > Fixes: d046f361dc93 ("Xen Security Modules: XSM")
> > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > ---
> > > The hook being able to deny access to data for certain domains means
> > > that no caller can assume to have a system-wide picture when holding the
> > > results.
> > >
> > > Wouldn't it make sense to permit the function to merely "count" domains?
> > > While racy in general (including in its present, "normal" mode of
> > > operation), within a tool stack this could be used as long as creation
> > > of new domains is suppressed between obtaining the count and then using
> > > it.
> > >
> > > In XEN_DOMCTL_getpageframeinfo2 said commit had introduced a 2nd such
> > > issue, but luckily that sub-op and xsm_getpageframeinfo() are long gone.
> > I understand there is a larger issue at play here but neutering the
> > security control/XSM check is not the answer. This literally changes the
> > way a FLASK policy that people currently have would be enforced, as well
> > as contrary to how they understand the access control that it provides.
> > Even though the code path does not fall under XSM maintainer, I would
> > NACK this patch. IMHO, it is better to find a solution that does not
> > abuse, misuse, or invalidate the purpose of the XSM calls.
> >
> > On a side note, I am a little concern that only one person thought to
> > include the XSM maintainer, or any of the XSM reviewers, onto a patch
> > and the discussion around a patch that clearly relates to XSM for us to
> > gauge the consequences of the patch. I am not assuming intentions here,
> > only wanting to raise the concern.
> Well, yes, for the discussion items I could have remembered to include
> you. The code change itself, otoh, doesn't require your ack, even if it
> is the return value of an XSM function which was used wrongly here.
I beg to disagree, not that you could have, but that you should have. 
This is now the second XSM issue, that I am aware of at least, that 
myself and the XSM reviewers have been left out of. How and where the 
XSM hooks are deployed are critical to how XSM function, regardless of 
how mundane the change may be. By your logic, as the XSM maintainer I 
can make changes to the XSM code that changes how the system behaves for 
x86 and claim you have no Ack/Nack authority since it is XSM code. These 
subsystems are symbiotic, and we owe each other the due respect to 
include to the other when these systems touch or influence each other.
> > So for what it is worth, NACK.
> I'm puzzled: I hope you don't mean NACK to the patch (or effectively
> Jürgen's identical one, which I had noticed only after sending mine).
> Yet beyond that I don't see anything here which could be NACKed. I've
> merely raised a couple of points for discussion.
I will comment on Jurgen's patch.