On Wed, Dec 03, 2014 at 05:50:51PM +0000, Andre Przywara wrote:

On 30/11/14 08:45, Christoffer Dall wrote:

quoted

On Fri, Nov 28, 2014 at 03:40:12PM +0000, Andre Przywara wrote:

quoted

Hej Christoffer,

On 25/11/14 11:03, Christoffer Dall wrote:

quoted

Hi Andre,

On Mon, Nov 24, 2014 at 04:37:58PM +0000, Andre Przywara wrote:

quoted

Hi,

On 23/11/14 15:08, Christoffer Dall wrote:

quoted

On Fri, Nov 14, 2014 at 10:08:01AM +0000, Andre Przywara wrote:

quoted

While the generation of a (virtual) inter-processor interrupt (SGI)
on a GICv2 works by writing to a MMIO register, GICv3 uses the system
register ICC_SGI1R_EL1 to trigger them.
Trap that register on ARM64 hosts and handle it in a new handler
function in the GICv3 emulation code.

Did you reorder something or does my previous comment still apply that
you're not enabling trapping yet, you're just adding the handler - those
are two different things.

Yes, I can fix the wording.

quoted

You sort of left my question about access_gic_sgi() not checking if the
gicv3 is presetn hanging from the last thread, but I think I'm
understanding properly now, that as long as you're not setting the
ICC_SRE_EL2.Enable = 1, then we'll never get here, right?

Right, that is the idea. Just to make sure that I got this right from
the discussion the other day: We will not trap to EL2 as long as
ICC_SRE_EL2.Enable is 0 - which it should still be at this point, right?

No, when ICC_SRE_EL2.Enable is 0, then Non-secure EL1 access to
ICC_SRE_EL1 trap to EL2 (See Section 5.7.39 in the spec), which means
that accesses to the ICC_SGIx registers will cause an undefined
exception in the guest because we set ICC_SRE_EL1.SRE to 0 for the
guest and the guest cannot change this.

Now, when we set ICC_SRE_EL2.Enable to 1, then the guest can set
ICC_SRE_EL1.SRE to 1 (and we also happen to reset it to 1), and we will
indeed trap on guest access to the ICC_SGIx registers, because all
virtual accesses to these registers trap.

(Going back and checking where 'virtual accesses' is defined in the spec
left me somewhere without any results, but I am guessing that because we
set the ICH_HCR_EL2.En to 1, all accesses will be deemed virtual
accesses, maybe the spec should be clarfied on this matter?).

Anyhow, to get back to my original question, getting here requires
a situation where the guest copy of the ICC_SRE_EL1.SRE is 1, which we
only allow when we have properly initialized the GICv3 data structures.

So to summarize (and check) this: There is no real issue at this point?
And the code is totally fine after 19/19?

There is no issue at this point, no.

quoted

Would this kind of problem actually matter _inside_ a patch series? To
trigger an issue, we would need a bogus guest and bogus userland
(because at this point neither of them would see/inject a GICv3 FDT
node). I'd assume that running a kernel at this point is just for
debugging/bisecting? Where you wouldn't care about every corner case of
execution?

The argument about bogus guests / fdts should *never* be considered in
the context of these discussions.  If we have code that looks like the
guest can kill the host, or do a NULL pointer dereference, then we need
to address it.

Your point about it being inside a patch series, sure, it's unlikely
that people will run this, but I'm reviewing this patch right now, and
honestly not considering how this changes in the subsequent patch.  For
this sort of thing, if we were leaving a gaping hole open, that would at
least require a clear note in the commit message on why we're doing it.

I see, makes sense.
So I thought about adding a line like this to the very beginning of
vgic_v3_dispatch_sgi(). This would cover all cases of spurious traps.
Does that sound useful as a security precaution (though unneeded as
described)?
Shall there be a warning before the return?

+	/* only valid for an initialized VGICv3 */
+	if (!vgic_initialized(kvm)  ||
+	    kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return;

If anything you should have a BUG_ON(), but especially when you've
tested this, it won't happen.

quoted

Hopefully you understood and agreed with my deduction about the various
SRE settings above though?

Yes, I got this. We are safe as long as ICC_SRE_EL1.SRE is 0, which is
true until patch 19/19 allows userland to request a GICv3 guest, which
will force it to 1.
I also tested this explicitly, starting with patch 17/19 (for the host
kernel) and going over the remaining two as well. Starting a guest with
GICv2 and accessing ICC_SRE_EL1 and ICC_SGI1R_EL1 from a custom module
inside the guest will always keep ICC_SRE_EL1.SRE to 0 (thanks to your
recent trap patch), accesses to ICC_SGI1R_EL1 provoke an #UNDEF
exception in the guest. The host was never bothered.
Creating a guest with a GICv3 was only successful after patch 19/19, and
ICC_SRE_EL1.SRE couldn't be cleared.

So I consider this topic done.

Indeed!
-Christoffer