| Message ID | 20241209110717.77279-1-schlameuss@linux.ibm.com |
|---|---|
| Headers | show |
| Series | selftests: kvm: s390: Reject invalid ioctls on ucontrol VMs | expand |
On 12/9/24 12:07 PM, Christoph Schlameuss wrote: > Prevent null pointer dereference when processing the > KVM_DEV_FLIC_APF_ENABLE and KVM_DEV_FLIC_APF_DISABLE_WAIT ioctls in the > interrupt controller. > > Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > Signed-off-by: Christoph Schlameuss <schlameuss@linux.ibm.com> Please add a fixes tag to this patch and #3. It's fine to just send a reply to the patches until there are enough comments for a proper v2.
On Mon Dec 9, 2024 at 7:14 PM CET, Claudio Imbrenda wrote: > On Mon, 9 Dec 2024 12:07:13 +0100 > Christoph Schlameuss <schlameuss@linux.ibm.com> wrote: > > > Add some superficial selftests for the floating interrupt controller > > when using ucontrol VMs. These tests are intended to cover very basic > > calls only. > > > > Some of the calls may trigger null pointer dereferences on kernels not > > containing the fixes in this patch series. > > > > Signed-off-by: Christoph Schlameuss <schlameuss@linux.ibm.com> > > --- > > .../selftests/kvm/s390x/ucontrol_test.c | 150 ++++++++++++++++++ > > 1 file changed, 150 insertions(+) > > > > diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c > > index 0c112319dab1..972fac1023b5 100644 > > --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c > > +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c > > @@ -635,4 +635,154 @@ TEST_F(uc_kvm, uc_skey) > > uc_assert_diag44(self); > > } > > > > +static char uc_flic_b[PAGE_SIZE]; > > +static struct kvm_s390_io_adapter uc_flic_ioa = { .id = 0 }; > > +static struct kvm_s390_io_adapter_req uc_flic_ioam = { .id = 0 }; > > +static struct kvm_s390_ais_req uc_flic_asim = { .isc = 0 }; > > +static struct kvm_s390_ais_all uc_flic_asima = { .simm = 0 }; > > +static struct uc_flic_attr_test { > > + char *name; > > + struct kvm_device_attr a; > > + int hasrc; > > + u64 getrc; > > + int geterrno; > > + u64 setrc; > > I wonder if you really need getrc and setrc? (see below) > > > + int seterrno; > > +} uc_flic_attr_tests[] = { > > + { > > + .name = "KVM_DEV_FLIC_GET_ALL_IRQS", > > + .setrc = 1, .seterrno = EINVAL, > > please put them on separate lines ^ (if you end up keeping both) > > > + .a = { > > + .group = KVM_DEV_FLIC_GET_ALL_IRQS, > > + .addr = (u64)&uc_flic_b, > > + .attr = PAGE_SIZE, > > + }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_ENQUEUE", > > + .getrc = 1, .geterrno = EINVAL, > > + .a = { .group = KVM_DEV_FLIC_ENQUEUE, }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_CLEAR_IRQS", > > + .getrc = 1, .geterrno = EINVAL, > > + .a = { .group = KVM_DEV_FLIC_CLEAR_IRQS, }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_ADAPTER_REGISTER", > > + .getrc = 1, .geterrno = EINVAL, > > + .a = { > > + .group = KVM_DEV_FLIC_ADAPTER_REGISTER, > > + .addr = (u64)&uc_flic_ioa, > > + }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_ADAPTER_MODIFY", > > + .getrc = 1, .geterrno = EINVAL, > > + .setrc = 1, .seterrno = EINVAL, > > + .a = { > > + .group = KVM_DEV_FLIC_ADAPTER_MODIFY, > > + .addr = (u64)&uc_flic_ioam, > > + .attr = sizeof(uc_flic_ioam), > > + }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_CLEAR_IO_IRQ", > > + .getrc = 1, .geterrno = EINVAL, > > + .setrc = 1, .seterrno = EINVAL, > > + .a = { > > + .group = KVM_DEV_FLIC_CLEAR_IO_IRQ, > > + .attr = 32, > > + }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_AISM", > > + .getrc = 1, .geterrno = EINVAL, > > + .setrc = 1, .seterrno = ENOTSUP, > > + .a = { > > + .group = KVM_DEV_FLIC_AISM, > > + .addr = (u64)&uc_flic_asim, > > + }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_AIRQ_INJECT", > > + .getrc = 1, .geterrno = EINVAL, > > + .a = { .group = KVM_DEV_FLIC_AIRQ_INJECT, }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_AISM_ALL", > > + .getrc = 1, .geterrno = ENOTSUP, > > + .setrc = 1, .seterrno = ENOTSUP, > > + .a = { > > + .group = KVM_DEV_FLIC_AISM_ALL, > > + .addr = (u64)&uc_flic_asima, > > + .attr = sizeof(uc_flic_asima), > > + }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_APF_ENABLE", > > + .getrc = 1, .geterrno = EINVAL, > > + .setrc = 1, .seterrno = EINVAL, > > + .a = { .group = KVM_DEV_FLIC_APF_ENABLE, }, > > + }, > > + { > > + .name = "KVM_DEV_FLIC_APF_DISABLE_WAIT", > > + .getrc = 1, .geterrno = EINVAL, > > + .setrc = 1, .seterrno = EINVAL, > > + .a = { .group = KVM_DEV_FLIC_APF_DISABLE_WAIT, }, > > + }, > > +}; > > + > > +TEST_F(uc_kvm, uc_flic_attrs) > > +{ > > + struct kvm_create_device cd = { .type = KVM_DEV_TYPE_FLIC }; > > + struct kvm_device_attr attr; > > + u64 value; > > + int rc, i; > > + > > + rc = ioctl(self->vm_fd, KVM_CREATE_DEVICE, &cd); > > + ASSERT_EQ(0, rc) TH_LOG("create device failed with err %s (%i)", > > + strerror(errno), errno); > > + > > + for (i = 0; i < ARRAY_SIZE(uc_flic_attr_tests); i++) { > > + TH_LOG("test %s", uc_flic_attr_tests[i].name); > > + attr = (struct kvm_device_attr) { > > + .group = uc_flic_attr_tests[i].a.group, > > + .attr = uc_flic_attr_tests[i].a.attr, > > + .addr = uc_flic_attr_tests[i].a.addr, > > + }; > > + if (attr.addr == 0) > > + attr.addr = (u64)&value; > > + > > + rc = ioctl(cd.fd, KVM_HAS_DEVICE_ATTR, &attr); > > + EXPECT_EQ(uc_flic_attr_tests[i].hasrc, !!rc) > > + TH_LOG("expected dev attr missing %s", > > + uc_flic_attr_tests[i].name); > > + > > + rc = ioctl(cd.fd, KVM_GET_DEVICE_ATTR, &attr); > > + EXPECT_EQ(uc_flic_attr_tests[i].getrc, !!rc) > > maybe you could just do: > > EXPECT_EQ(!!uc_flic_attr_tests[i].geterrno, !!rc) > > (unless I am missing something) > > this is not super important, though > Yes, that should work. I will do that. Thanks! > > + TH_LOG("get dev attr rc not expected on %s %s (%i)", > > + uc_flic_attr_tests[i].name, > > + strerror(errno), errno); > > + if (uc_flic_attr_tests[i].geterrno) > > + EXPECT_EQ(uc_flic_attr_tests[i].geterrno, errno) > > + TH_LOG("get dev attr errno not expected on %s %s (%i)", > > + uc_flic_attr_tests[i].name, > > + strerror(errno), errno); > > + > > + rc = ioctl(cd.fd, KVM_SET_DEVICE_ATTR, &attr); > > + EXPECT_EQ(uc_flic_attr_tests[i].setrc, !!rc) > > + TH_LOG("set sev attr rc not expected on %s %s (%i)", > > + uc_flic_attr_tests[i].name, > > + strerror(errno), errno); > > + if (uc_flic_attr_tests[i].seterrno) > > + EXPECT_EQ(uc_flic_attr_tests[i].seterrno, errno) > > + TH_LOG("set dev attr errno not expected on %s %s (%i)", > > + uc_flic_attr_tests[i].name, > > + strerror(errno), errno); > > + } > > + > > + close(cd.fd); > > +} > > + > > TEST_HARNESS_MAIN
This patch series adds more test case issuing ioctls to ucontrol VMs and its floating interrupt controller. The test cases trigger three possible null pointer dereferences within the handling of the KVM_DEV_FLIC_APF_ENABLE, KVM_DEV_FLIC_APF_DISABLE_WAIT and KVM_SET_GSI_ROUTING ioctl. All of these issues do only exist on ucontrol VMs. Fixes for the issues are included within the patch series. Christoph Schlameuss (6): kvm: s390: Reject setting flic pfault attributes on ucontrol VMs selftests: kvm: s390: Add ucontrol flic attr selftests kvm: s390: Reject KVM_SET_GSI_ROUTING on ucontrol VMs selftests: kvm: s390: Add ucontrol gis routing test selftests: kvm: s390: Streamline uc_skey test to issue iske after sske selftests: kvm: s390: Add has device attr check to uc_attr_mem_limit selftest arch/s390/kvm/interrupt.c | 6 + .../selftests/kvm/s390x/ucontrol_test.c | 196 ++++++++++++++++-- 2 files changed, 184 insertions(+), 18 deletions(-)