diff mbox series

[v2,2/4] KVM: s390: vsie: Fix length of facility list shadowed

Message ID 20231107123118.778364-3-nsg@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series KVM: s390: Fix minor bugs in STFLE shadowing | expand

Commit Message

Nina Schoetterl-Glausch Nov. 7, 2023, 12:31 p.m. UTC 
The length of the facility list accessed when interpretively executing
STFLE is the same as the hosts facility list (in case of format-0)
When shadowing, copy only those bytes.
The memory following the facility list need not be accessible, in which
case we'd wrongly inject a validity intercept.

Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 arch/s390/include/asm/facility.h |  6 ++++++
 arch/s390/kernel/Makefile        |  2 +-
 arch/s390/kernel/facility.c      | 21 +++++++++++++++++++++
 arch/s390/kvm/vsie.c             | 12 +++++++++++-
 4 files changed, 39 insertions(+), 2 deletions(-)
 create mode 100644 arch/s390/kernel/facility.c

Comments

Claudio Imbrenda Nov. 7, 2023, 5:11 p.m. UTC | #1 
On Tue,  7 Nov 2023 13:31:16 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

[...]

> -obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
> +obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o facility.o
>  
>  extra-y				+= vmlinux.lds
>  
> diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> new file mode 100644
> index 000000000000..5e80a4f65363
> --- /dev/null
> +++ b/arch/s390/kernel/facility.c

I wonder if this is the right place for this?

This function seems to be used only for vsie, maybe you can just move
it to vsie.c? or do you think it will be used elsewhere too?

[...]
Nina Schoetterl-Glausch Nov. 8, 2023, 10:30 a.m. UTC | #2 
On Tue, 2023-11-07 at 18:11 +0100, Claudio Imbrenda wrote:
> On Tue,  7 Nov 2023 13:31:16 +0100
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> 
> [...]
> 
> > -obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
> > +obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o facility.o
> >  
> >  extra-y				+= vmlinux.lds
> >  
> > diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> > new file mode 100644
> > index 000000000000..5e80a4f65363
> > --- /dev/null
> > +++ b/arch/s390/kernel/facility.c
> 
> I wonder if this is the right place for this?

I've wondered the same :D
> 
> This function seems to be used only for vsie, maybe you can just move
> it to vsie.c? or do you think it will be used elsewhere too?

It's a general STFLE function and if I put it into vsie.c I'm not sure
that, if the same functionality was required somewhere else, it would be
found and moved to a common location.

I was also somewhat resistant to calling a double underscore function from
vsie.c. Of course I could implement it with my own inline asm...

The way I did it seemed nicest, but if someone else has a strong opinion
I'll defer to that.
> 
> [...]
Heiko Carstens Nov. 8, 2023, 11:06 a.m. UTC | #3 
On Wed, Nov 08, 2023 at 11:30:09AM +0100, Nina Schoetterl-Glausch wrote:
> On Tue, 2023-11-07 at 18:11 +0100, Claudio Imbrenda wrote:
> > On Tue,  7 Nov 2023 13:31:16 +0100
> > Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> > 
> > [...]
> > 
> > > -obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
> > > +obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o facility.o
> > >  
> > >  extra-y				+= vmlinux.lds
> > >  
> > > diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> > > new file mode 100644
> > > index 000000000000..5e80a4f65363
> > > --- /dev/null
> > > +++ b/arch/s390/kernel/facility.c
> > 
> > I wonder if this is the right place for this?
> 
> I've wondered the same :D
> > 
> > This function seems to be used only for vsie, maybe you can just move
> > it to vsie.c? or do you think it will be used elsewhere too?
> 
> It's a general STFLE function and if I put it into vsie.c I'm not sure
> that, if the same functionality was required somewhere else, it would be
> found and moved to a common location.
> 
> I was also somewhat resistant to calling a double underscore function from
> vsie.c. Of course I could implement it with my own inline asm...
> 
> The way I did it seemed nicest, but if someone else has a strong opinion
> I'll defer to that.

I think it is ok to have new file for just this. It is better than what
we've done too often in the past: dump new functionality to some more or
less random file instead. The usual victim would have been setup.c.

So I prefer a new file, even if we end up with only one function there.
Claudio Imbrenda Nov. 8, 2023, 11:23 a.m. UTC | #4 
On Tue,  7 Nov 2023 13:31:16 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

[...]

> diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> new file mode 100644
> index 000000000000..5e80a4f65363
> --- /dev/null
> +++ b/arch/s390/kernel/facility.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright IBM Corp. 2023
> + */
> +
> +#include <asm/facility.h>
> +
> +unsigned int stfle_size(void)
> +{
> +	static unsigned int size;
> +	u64 dummy;
> +	unsigned int r;

reverse Christmas tree please :)


with that fixed:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

[...]
Nina Schoetterl-Glausch Nov. 8, 2023, 11:49 a.m. UTC | #5 
On Wed, 2023-11-08 at 12:23 +0100, Claudio Imbrenda wrote:
> On Tue,  7 Nov 2023 13:31:16 +0100
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> 
> [...]
> 
> > diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> > new file mode 100644
> > index 000000000000..5e80a4f65363
> > --- /dev/null
> > +++ b/arch/s390/kernel/facility.c
> > @@ -0,0 +1,21 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright IBM Corp. 2023
> > + */
> > +
> > +#include <asm/facility.h>
> > +
> > +unsigned int stfle_size(void)
> > +{
> > +	static unsigned int size;
> > +	u64 dummy;
> > +	unsigned int r;
> 
> reverse Christmas tree please :)

Might be an opportunity to clear that up for me.
AFAIK reverse christmas tree isn't universally enforced in the kernel.
Do we do it in generic s390 code? I know we do for s390 kvm.
Personally I don't quite get the rational, but I don't care much either :)
Heiko?

> with that fixed:
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> [...]
Heiko Carstens Nov. 8, 2023, 3:52 p.m. UTC | #6 
On Wed, Nov 08, 2023 at 12:49:21PM +0100, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-11-08 at 12:23 +0100, Claudio Imbrenda wrote:
> > On Tue,  7 Nov 2023 13:31:16 +0100
> > Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> > 
> > [...]
> > > +unsigned int stfle_size(void)
> > > +{
> > > +	static unsigned int size;
> > > +	u64 dummy;
> > > +	unsigned int r;
> > 
> > reverse Christmas tree please :)
> 
> Might be an opportunity to clear that up for me.
> AFAIK reverse christmas tree isn't universally enforced in the kernel.
> Do we do it in generic s390 code? I know we do for s390 kvm.
> Personally I don't quite get the rational, but I don't care much either :)
> Heiko?

We do that for _new_ code in s390 code, yes.
diff mbox series

Patch

diff --git a/arch/s390/include/asm/facility.h b/arch/s390/include/asm/facility.h
index 94b6919026df..796007125dff 100644
--- a/arch/s390/include/asm/facility.h
+++ b/arch/s390/include/asm/facility.h
@@ -111,4 +111,10 @@  static inline void stfle(u64 *stfle_fac_list, int size)
 	preempt_enable();
 }
 
+/**
+ * stfle_size - Actual size of the facility list as specified by stfle
+ * (number of double words)
+ */
+unsigned int stfle_size(void);
+
 #endif /* __ASM_FACILITY_H */
diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
index 0df2b88cc0da..0daa81439478 100644
--- a/arch/s390/kernel/Makefile
+++ b/arch/s390/kernel/Makefile
@@ -41,7 +41,7 @@  obj-y	+= sysinfo.o lgr.o os_info.o
 obj-y	+= runtime_instr.o cache.o fpu.o dumpstack.o guarded_storage.o sthyi.o
 obj-y	+= entry.o reipl.o kdebugfs.o alternative.o
 obj-y	+= nospec-branch.o ipl_vmparm.o machine_kexec_reloc.o unwind_bc.o
-obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
+obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o facility.o
 
 extra-y				+= vmlinux.lds
 
diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
new file mode 100644
index 000000000000..5e80a4f65363
--- /dev/null
+++ b/arch/s390/kernel/facility.c
@@ -0,0 +1,21 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright IBM Corp. 2023
+ */
+
+#include <asm/facility.h>
+
+unsigned int stfle_size(void)
+{
+	static unsigned int size;
+	u64 dummy;
+	unsigned int r;
+
+	r = READ_ONCE(size);
+	if (!r) {
+		r = __stfle_asm(&dummy, 1) + 1;
+		WRITE_ONCE(size, r);
+	}
+	return r;
+}
+EXPORT_SYMBOL(stfle_size);
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index d989772fe211..c168e88c64da 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -19,6 +19,7 @@ 
 #include <asm/nmi.h>
 #include <asm/dis.h>
 #include <asm/fpu/api.h>
+#include <asm/facility.h>
 #include "kvm-s390.h"
 #include "gaccess.h"
 
@@ -990,11 +991,20 @@  static int handle_stfle(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
 	__u32 fac = READ_ONCE(vsie_page->scb_o->fac);
 
+	/*
+	 * Alternate-STFLE-Interpretive-Execution facilities are not supported
+	 * -> format-0 flcb
+	 */
 	if (fac && test_kvm_facility(vcpu->kvm, 7)) {
 		fac = fac & 0x7ffffff8U;
 		retry_vsie_icpt(vsie_page);
+		/*
+		 * format-0 -> size of nested guest's facility list == guest's size
+		 * guest's size == host's size, since STFLE is interpretatively executed
+		 * using a format-0 for the guest, too.
+		 */
 		if (read_guest_real(vcpu, fac, &vsie_page->fac,
-				    sizeof(vsie_page->fac)))
+				    stfle_size() * sizeof(u64)))
 			return set_validity_icpt(scb_s, 0x1090U);
 		scb_s->fac = (__u32)(__u64) &vsie_page->fac;
 	}