| Message ID | 147644021476.15715.14173453351089636671.stgit@Palanthas (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 14.10.16 at 12:17, <dario.faggioli@citrix.com> wrote: > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> I have to admit that I would have expected a non-empty description, at least briefly rationalizing the change of state. Jan
On 14/10/16 11:17, Dario Faggioli wrote:
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
We don't have a general framework for declaring things "supported" yet,
and at the moment we only have a single level of "supported", which
includes XSAs. I do think it makes sense to include an assessment of
the security risk when deciding whether to make such a declaration.
Here's a sort of checklist we could use to start a discussion.
Basically, there are a number of types of security vulnerabilities; we
could ask what the likelihood of any of these happening.
1. Guest -> host privilege escalation. What functionality do
unprivileged guest have access to -- hypercalls / instructions? Do
these include any buffers or pointers, and are these handled in a safe
manner -- not only checked, but with a "defensive programming" attitude
in mind? An informal audit of any such interfaces should generally
suffice to answer this question I think.
2. Guest user -> guest kernel escalation. Is there functionality
exposed to the guest kernel that must be limited to the kernel? If so,
is it properly secured from guest user mode? Might this feature have
any impact on any *other* functionality that the guest kernel uses to
protect itself from the guest user space?
3. Information leakage. What additional information do the interfaces
expose to anything that has access to them? Are there any buffers that
may copy hypervisor stack frames, &c?
4. Denial-of-service. Can anyone use any of the access they have to
crash the hypervisor, or monopolize resources in a way that
significantly impedes the performance of other guests, or other
processes within the same guest?
---
WRT Credit2:
I *think* the only interfaces exposed to *unprivileged* guests are the
SCHEDOP hypercalls -- block(), yield(), &c -- and timers. (Dario,
please correct me if I'm wrong.) The only thing that the scheduler
gives is time on the cpu. Neither block nor yield contain any pointers,
and so it should be trivial to verify that they contain no privilege
escalation paths.
WRT guest user -> guest kernel escalation, the guest kernel is not
relying on anything from the scheduler to protect itself or any data in
any way, so it's difficult to imagine how this might be done.
The only information which the scheduler exposes to unprivileged guests
is the timing information. This may be able to be used for side-channel
attacks to probabilistically infer things about other vcpus running on
the same system; but this has not traditionally been considered within
the security boundary.
There have been denial-of-service attacks on credit1 before, where a
vcpu could "game the system" by going to sleep at particular times to
fool the accounting system into thinking that it wasn't running, and
thus run at BOOST priority and monopolize 95% of the cpu. But this was
fixed by actually charging cpus for time they spent running rather than
probabilistycally, which credit2 already does.
It's worth taking stock again and thinking if there's any way to "game"
credit2 in such a way; but I think it's relatively unlikely that someone
will be able to monopolize the cpu 100% as with the credit1 bug; and
even if it did, it's a pretty low-profile XSA.
So on the whole, I'm inclined to say that from a security perspective,
credit2 is probably OK. (It would help if Dario filled this out a bit,
I think.)
But I agree with Jan, that such an argument would go well to go in the
commit message. :-)
-George
On Fri, 2016-10-14 at 11:42 +0100, George Dunlap wrote: > On 14/10/16 11:17, Dario Faggioli wrote: > > > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > > We don't have a general framework for declaring things "supported" > yet, > and at the moment we only have a single level of "supported", which > includes XSAs. I do think it makes sense to include an assessment of > the security risk when deciding whether to make such a declaration. > Right... So, first of all, sorry if I dropped the ball on this for a while, but I really wasn't sure of what should have come next (more people from security team speaking their mind? Here or somewhere else? Etc.) As already said by others, it's hard to follow the steps of a procedure which is, in fact, unknown and not formalized nor written down anywhere (which, literally, _just_ mean we need to formalize and write down it! :-D). > Here's a sort of checklist we could use to start a discussion. > Ok, thanks George. I'll go through the checklist, and your analysis of Credit2, and provide my view (when worthwhile). > 1. Guest -> host privilege escalation. > 2. Guest user -> guest kernel escalation. > 3. Information leakage. > 4. Denial-of-service. > > --- > > WRT Credit2: > > I *think* the only interfaces exposed to *unprivileged* guests are > the > SCHEDOP hypercalls -- block(), yield(), &c -- and timers. (Dario, > please correct me if I'm wrong.) > I agree, that's all it's there, especially if we are talking about unprivileged guests. If we include the control domain, we also expose: - XEN_DOMCTL_SCHEDOP_* hypercalls, - XEN_SYSCTL_SCHEDOP_* hypercalls. > The only thing that the scheduler > gives is time on the cpu. Neither block nor yield contain any > pointers, > and so it should be trivial to verify that they contain no privilege > escalation paths. > I agree. FWIW, I've tried auditing that code, and could not find anything that looks like a potential security issue. E.g., there's not buffer/local variable, being used, that could lead to hypervisor stack leaks (and in fact, this is probably more about George's point 3 than 1). > The only information which the scheduler exposes to unprivileged > guests > is the timing information. This may be able to be used for side- > channel > attacks to probabilistically infer things about other vcpus running > on > the same system; but this has not traditionally been considered > within > the security boundary. > And this is possible with all schedulers. Not that we can't think of improvements, of course, but if this counts, Credit1 is *Experimental* too. :-) > There have been denial-of-service attacks on credit1 before, where a > vcpu could "game the system" by going to sleep at particular times to > fool the accounting system into thinking that it wasn't running, and > thus run at BOOST priority and monopolize 95% of the cpu. But this > was > fixed by actually charging cpus for time they spent running rather > than > probabilistycally, which credit2 already does. > And Credit2 does not have BOOST or, said in a more general way, it does not have any way for a vcpu to acquire any kind of "superpowers"... they're all subjected to the same crediting algorithm, which is a lot simpler than Credit1's one, and hence a lot easier to understand, debug and audit. > It's worth taking stock again and thinking if there's any way to > "game" > credit2 in such a way; but I think it's relatively unlikely that > someone > will be able to monopolize the cpu 100% as with the credit1 bug; and > even if it did, it's a pretty low-profile XSA. > It may be me not being a 'security guy' (yet :-)), but I can't think of anything. > But I agree with Jan, that such an argument would go well to go in > the > commit message. :-) > Right. Actually, I wrote a few words about a lot of development and testing having been done lately, and I thought whether or not to use that as commit message. I certainly could have thought about doing something like this that George did, but: - I probably wouldn't have been able to do it as good as him; - I wasn't sure whether it was worth/meaningful for me to do it, as opposed to the security team wanting to do that themselves. So, again, let's formalize the process, because it's hard to follow an unwritten and implicite set of rules. :-) All this being said, what's the next step? Is there anything more I should do, here in this thread? Should we wait for other people to weigh in? Should I resend this patch, with a non empty commit message? Containing what, a summary of this? Any help and insight, would be greatly appreciated. Thanks for everyone's time and Regards, Dario
On 25/10/16 09:18, Dario Faggioli wrote: > On Fri, 2016-10-14 at 11:42 +0100, George Dunlap wrote: >> On 14/10/16 11:17, Dario Faggioli wrote: >>> >>> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> >> >> We don't have a general framework for declaring things "supported" >> yet, >> and at the moment we only have a single level of "supported", which >> includes XSAs. I do think it makes sense to include an assessment of >> the security risk when deciding whether to make such a declaration. >> > Right... So, first of all, sorry if I dropped the ball on this for a > while, but I really wasn't sure of what should have come next (more > people from security team speaking their mind? Here or somewhere else? > Etc.) > > As already said by others, it's hard to follow the steps of a procedure > which is, in fact, unknown and not formalized nor written down anywhere > (which, literally, _just_ mean we need to formalize and write down it! > :-D). > >> Here's a sort of checklist we could use to start a discussion. >> > Ok, thanks George. I'll go through the checklist, and your analysis of > Credit2, and provide my view (when worthwhile). > >> 1. Guest -> host privilege escalation. >> 2. Guest user -> guest kernel escalation. >> 3. Information leakage. >> 4. Denial-of-service. >> >> --- >> >> WRT Credit2: >> >> I *think* the only interfaces exposed to *unprivileged* guests are >> the >> SCHEDOP hypercalls -- block(), yield(), &c -- and timers. (Dario, >> please correct me if I'm wrong.) >> > I agree, that's all it's there, especially if we are talking about > unprivileged guests. If we include the control domain, we also expose: > - XEN_DOMCTL_SCHEDOP_* hypercalls, > - XEN_SYSCTL_SCHEDOP_* hypercalls. > >> The only thing that the scheduler >> gives is time on the cpu. Neither block nor yield contain any >> pointers, >> and so it should be trivial to verify that they contain no privilege >> escalation paths. >> > I agree. > > FWIW, I've tried auditing that code, and could not find anything that > looks like a potential security issue. E.g., there's not buffer/local > variable, being used, that could lead to hypervisor stack leaks (and in > fact, this is probably more about George's point 3 than 1). > >> The only information which the scheduler exposes to unprivileged >> guests >> is the timing information. This may be able to be used for side- >> channel >> attacks to probabilistically infer things about other vcpus running >> on >> the same system; but this has not traditionally been considered >> within >> the security boundary. >> > And this is possible with all schedulers. Not that we can't think of > improvements, of course, but if this counts, Credit1 is *Experimental* > too. :-) > >> There have been denial-of-service attacks on credit1 before, where a >> vcpu could "game the system" by going to sleep at particular times to >> fool the accounting system into thinking that it wasn't running, and >> thus run at BOOST priority and monopolize 95% of the cpu. But this >> was >> fixed by actually charging cpus for time they spent running rather >> than >> probabilistycally, which credit2 already does. >> > And Credit2 does not have BOOST or, said in a more general way, it does > not have any way for a vcpu to acquire any kind of "superpowers"... > they're all subjected to the same crediting algorithm, which is a lot > simpler than Credit1's one, and hence a lot easier to understand, debug > and audit. > >> It's worth taking stock again and thinking if there's any way to >> "game" >> credit2 in such a way; but I think it's relatively unlikely that >> someone >> will be able to monopolize the cpu 100% as with the credit1 bug; and >> even if it did, it's a pretty low-profile XSA. >> > It may be me not being a 'security guy' (yet :-)), but I can't think of > anything. > >> But I agree with Jan, that such an argument would go well to go in >> the >> commit message. :-) >> > Right. Actually, I wrote a few words about a lot of development and > testing having been done lately, and I thought whether or not to use > that as commit message. I certainly could have thought about doing > something like this that George did, but: > - I probably wouldn't have been able to do it as good as him; > - I wasn't sure whether it was worth/meaningful for me to do it, as > opposed to the security team wanting to do that themselves. > > So, again, let's formalize the process, because it's hard to follow an > unwritten and implicite set of rules. :-) > > All this being said, what's the next step? Is there anything more I > should do, here in this thread? Should we wait for other people to > weigh in? Should I resend this patch, with a non empty commit message? > Containing what, a summary of this? Well I think that in the absence of official rules, we have to make things up as we go along. Since Andrew has raised the issue of security, I think basically there needs to be a reasonable justification that the feature is both ready from a support / reliability point of view, and ready from a security point of view. I've outlined what kinds of criteria we should be looking at for a security point of view, and made an example case for the credit scheduler. I think to get this enabled now requires someone to send a patch with the justification (both from a support/reliability standpoint and from a security standpoint) in the commit message. If we get a REST Maintainer's Ack and no objections, then it should go in (subject to the release coodinator, of course). Do you want to send such a patch, or would you prefer it if I tried to write something up? -George
On Thu, 2016-10-27 at 16:01 +0100, George Dunlap wrote: > On 25/10/16 09:18, Dario Faggioli wrote: > > All this being said, what's the next step? Is there anything more I > > should do, here in this thread? Should we wait for other people to > > weigh in? Should I resend this patch, with a non empty commit > > message? > > Containing what, a summary of this? > > Well I think that in the absence of official rules, we have to make > things up as we go along. > Fine with me. :-) > I think to get this enabled now requires someone to send a patch with > the justification (both from a support/reliability standpoint and > from a > security standpoint) in the commit message. If we get a REST > Maintainer's Ack and no objections, then it should go in (subject to > the > release coodinator, of course). > Ok. > Do you want to send such a patch, or would you prefer it if I tried > to > write something up? > I'd like to give it a try myself. Thanks and Regards, Dario
[Cc-list trimmed] On Thu, 2016-10-27 at 16:01 +0100, George Dunlap wrote: > On 25/10/16 09:18, Dario Faggioli wrote: > Do you want to send such a patch, or would you prefer it if I tried > to > write something up? > Here: https://lists.xen.org/archives/html/xen-devel/2016-11/msg00144.html For the security part, I 'borrowed' most of your arguments (and, sometimes, even your words) from your email in this thread... I hope that's fine. :-) Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
diff --git a/docs/features/sched_credit2.pandoc b/docs/features/sched_credit2.pandoc index 8609d9c..9c8e15b 100644 --- a/docs/features/sched_credit2.pandoc +++ b/docs/features/sched_credit2.pandoc @@ -5,7 +5,7 @@ # Basics ---------------- ---------------------------------------------------- - Status: **Experimental** + Status: **Supported** Component: Hypervisor ---------------- ---------------------------------------------------- diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index fe46e80..1f26553 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -2954,8 +2954,6 @@ csched2_init(struct scheduler *ops) struct csched2_private *prv; printk("Initializing Credit2 scheduler\n"); - printk(" WARNING: This is experimental software in development.\n" \ - " Use at your own risk.\n"); printk(XENLOG_INFO " load_precision_shift: %d\n" XENLOG_INFO " load_window_shift: %d\n"
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- This goes on top of: https://lists.xen.org/archives/html/xen-devel/2016-10/msg00944.html I'm of course happy to rebase/resend, if the above mentioned series changes. --- Cc: security@xenproject.org Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Anshul Makkar <anshul.makkar@citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Lars Kurth <lars.kurth@citrix.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> --- docs/features/sched_credit2.pandoc | 2 +- xen/common/sched_credit2.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)