* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-12 9:20 ` Thomas Huth
@ 2019-09-12 9:28 ` Janosch Frank
2019-09-12 10:52 ` Christian Borntraeger
2019-09-12 10:58 ` David Hildenbrand
2 siblings, 0 replies; 15+ messages in thread
From: Janosch Frank @ 2019-09-12 9:28 UTC (permalink / raw)
To: Thomas Huth, David Hildenbrand, Christian Borntraeger, kvm
Cc: Cornelia Huck, linux-s390, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2951 bytes --]
On 9/12/19 11:20 AM, Thomas Huth wrote:
> On 12/09/2019 11.14, David Hildenbrand wrote:
>> On 12.09.19 11:00, Thomas Huth wrote:
>>> When the userspace program runs the KVM_S390_INTERRUPT ioctl to inject
>>> an interrupt, we convert them from the legacy struct kvm_s390_interrupt
>>> to the new struct kvm_s390_irq via the s390int_to_s390irq() function.
>>> However, this function does not take care of all types of interrupts
>>> that we can inject into the guest later (see do_inject_vcpu()). Since we
>>> do not clear out the s390irq values before calling s390int_to_s390irq(),
>>> there is a chance that we copy unwanted data from the kernel stack
>>> into the guest memory later if the interrupt data has not been properly
>>> initialized by s390int_to_s390irq().
>>>
>>> Specifically, the problem exists with the KVM_S390_INT_PFAULT_INIT
>>> interrupt: s390int_to_s390irq() does not handle it, but the function
>>> __deliver_pfault_init() will later copy the uninitialized stack data
>>> from the ext.ext_params2 into the guest memory.
>>>
>>> Fix it by handling that interrupt type in s390int_to_s390irq(), too.
>>> And while we're at it, make sure that s390int_to_s390irq() now
>>> directly returns -EINVAL for unknown interrupt types, so that we
>>> do not run into this problem again in case we add more interrupt
>>> types to do_inject_vcpu() sometime in the future.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> arch/s390/kvm/interrupt.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index 3e7efdd9228a..165dea4c7f19 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -1960,6 +1960,16 @@ int s390int_to_s390irq(struct kvm_s390_interrupt *s390int,
>>> case KVM_S390_MCHK:
>>> irq->u.mchk.mcic = s390int->parm64;
>>> break;
>>> + case KVM_S390_INT_PFAULT_INIT:
>>> + irq->u.ext.ext_params = s390int->parm;
>>> + irq->u.ext.ext_params2 = s390int->parm64;
>>> + break;
>>> + case KVM_S390_RESTART:
>>> + case KVM_S390_INT_CLOCK_COMP:
>>> + case KVM_S390_INT_CPU_TIMER:
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> }
>>> return 0;
>>> }
>>>
>>
>> Wouldn't a safe fix be to initialize the struct to zero in the caller?
>
> That's of course possible, too. But that means that we always have to
> zero out the whole structure, so that's a little bit more of overhead
> (well, it likely doesn't matter for such a legacy ioctl).
Or memset it in s390int_to_s390irq so the caller doesn't need to know.
That should be fast enough for such a small struct.
>
> But the more important question: Do we then still care of fixing the
> PFAULT_INIT interrupt here? Since it requires a parameter, the "case
> KVM_S390_INT_PFAULT_INIT:" part would be required here anyway.
Let's wait for Christian's opinion on that.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-12 9:20 ` Thomas Huth
2019-09-12 9:28 ` Janosch Frank
@ 2019-09-12 10:52 ` Christian Borntraeger
2019-09-12 11:23 ` Thomas Huth
2019-09-12 10:58 ` David Hildenbrand
2 siblings, 1 reply; 15+ messages in thread
From: Christian Borntraeger @ 2019-09-12 10:52 UTC (permalink / raw)
To: Thomas Huth, David Hildenbrand, Janosch Frank, kvm
Cc: Cornelia Huck, linux-s390, linux-kernel
On 12.09.19 11:20, Thomas Huth wrote:
> On 12/09/2019 11.14, David Hildenbrand wrote:
>> On 12.09.19 11:00, Thomas Huth wrote:
>>> When the userspace program runs the KVM_S390_INTERRUPT ioctl to inject
>>> an interrupt, we convert them from the legacy struct kvm_s390_interrupt
>>> to the new struct kvm_s390_irq via the s390int_to_s390irq() function.
>>> However, this function does not take care of all types of interrupts
>>> that we can inject into the guest later (see do_inject_vcpu()). Since we
>>> do not clear out the s390irq values before calling s390int_to_s390irq(),
>>> there is a chance that we copy unwanted data from the kernel stack
>>> into the guest memory later if the interrupt data has not been properly
>>> initialized by s390int_to_s390irq().
>>>
>>> Specifically, the problem exists with the KVM_S390_INT_PFAULT_INIT
>>> interrupt: s390int_to_s390irq() does not handle it, but the function
>>> __deliver_pfault_init() will later copy the uninitialized stack data
>>> from the ext.ext_params2 into the guest memory.
>>>
>>> Fix it by handling that interrupt type in s390int_to_s390irq(), too.
>>> And while we're at it, make sure that s390int_to_s390irq() now
>>> directly returns -EINVAL for unknown interrupt types, so that we
>>> do not run into this problem again in case we add more interrupt
>>> types to do_inject_vcpu() sometime in the future.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> arch/s390/kvm/interrupt.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index 3e7efdd9228a..165dea4c7f19 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -1960,6 +1960,16 @@ int s390int_to_s390irq(struct kvm_s390_interrupt *s390int,
>>> case KVM_S390_MCHK:
>>> irq->u.mchk.mcic = s390int->parm64;
>>> break;
>>> + case KVM_S390_INT_PFAULT_INIT:
>>> + irq->u.ext.ext_params = s390int->parm;
>>> + irq->u.ext.ext_params2 = s390int->parm64;
>>> + break;
>>> + case KVM_S390_RESTART:
>>> + case KVM_S390_INT_CLOCK_COMP:
>>> + case KVM_S390_INT_CPU_TIMER:
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> }
>>> return 0;
>>> }
>>>
>>
>> Wouldn't a safe fix be to initialize the struct to zero in the caller?
>
> That's of course possible, too. But that means that we always have to
> zero out the whole structure, so that's a little bit more of overhead
> (well, it likely doesn't matter for such a legacy ioctl).
Yes doing something like
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index c19a24e940a1..b1f6f434af5d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4332,7 +4332,7 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
}
case KVM_S390_INTERRUPT: {
struct kvm_s390_interrupt s390int;
- struct kvm_s390_irq s390irq;
+ struct kvm_s390_irq s390irq = {};
if (copy_from_user(&s390int, argp, sizeof(s390int)))
return -EFAULT;
would certainly be ok as well, but
> But the more important question: Do we then still care of fixing the
> PFAULT_INIT interrupt here? Since it requires a parameter, the "case
> KVM_S390_INT_PFAULT_INIT:" part would be required here anyway.
as long as we we this interface we should fix it and we should do the
pfault thing correctly.
Maybe we should start to deprecate this interface and remove it.
For the time being Thomas fix is certainly good enough. We might want to
add the designated initializer as an additional safety barrier.
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-12 10:52 ` Christian Borntraeger
@ 2019-09-12 11:23 ` Thomas Huth
2019-09-13 7:20 ` Cornelia Huck
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Huth @ 2019-09-12 11:23 UTC (permalink / raw)
To: Christian Borntraeger, David Hildenbrand, Janosch Frank, kvm
Cc: Cornelia Huck, linux-s390, linux-kernel
On 12/09/2019 12.52, Christian Borntraeger wrote:
>
>
> On 12.09.19 11:20, Thomas Huth wrote:
>> On 12/09/2019 11.14, David Hildenbrand wrote:
>>> On 12.09.19 11:00, Thomas Huth wrote:
>>>> When the userspace program runs the KVM_S390_INTERRUPT ioctl to inject
>>>> an interrupt, we convert them from the legacy struct kvm_s390_interrupt
>>>> to the new struct kvm_s390_irq via the s390int_to_s390irq() function.
>>>> However, this function does not take care of all types of interrupts
>>>> that we can inject into the guest later (see do_inject_vcpu()). Since we
>>>> do not clear out the s390irq values before calling s390int_to_s390irq(),
>>>> there is a chance that we copy unwanted data from the kernel stack
>>>> into the guest memory later if the interrupt data has not been properly
>>>> initialized by s390int_to_s390irq().
>>>>
>>>> Specifically, the problem exists with the KVM_S390_INT_PFAULT_INIT
>>>> interrupt: s390int_to_s390irq() does not handle it, but the function
>>>> __deliver_pfault_init() will later copy the uninitialized stack data
>>>> from the ext.ext_params2 into the guest memory.
>>>>
>>>> Fix it by handling that interrupt type in s390int_to_s390irq(), too.
>>>> And while we're at it, make sure that s390int_to_s390irq() now
>>>> directly returns -EINVAL for unknown interrupt types, so that we
>>>> do not run into this problem again in case we add more interrupt
>>>> types to do_inject_vcpu() sometime in the future.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> arch/s390/kvm/interrupt.c | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>> index 3e7efdd9228a..165dea4c7f19 100644
>>>> --- a/arch/s390/kvm/interrupt.c
>>>> +++ b/arch/s390/kvm/interrupt.c
>>>> @@ -1960,6 +1960,16 @@ int s390int_to_s390irq(struct kvm_s390_interrupt *s390int,
>>>> case KVM_S390_MCHK:
>>>> irq->u.mchk.mcic = s390int->parm64;
>>>> break;
>>>> + case KVM_S390_INT_PFAULT_INIT:
>>>> + irq->u.ext.ext_params = s390int->parm;
>>>> + irq->u.ext.ext_params2 = s390int->parm64;
>>>> + break;
>>>> + case KVM_S390_RESTART:
>>>> + case KVM_S390_INT_CLOCK_COMP:
>>>> + case KVM_S390_INT_CPU_TIMER:
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>>> }
>>>> return 0;
>>>> }
>>>>
>>>
>>> Wouldn't a safe fix be to initialize the struct to zero in the caller?
>>
>> That's of course possible, too. But that means that we always have to
>> zero out the whole structure, so that's a little bit more of overhead
>> (well, it likely doesn't matter for such a legacy ioctl).
>
> Yes doing something like
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index c19a24e940a1..b1f6f434af5d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4332,7 +4332,7 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
> }
> case KVM_S390_INTERRUPT: {
> struct kvm_s390_interrupt s390int;
> - struct kvm_s390_irq s390irq;
> + struct kvm_s390_irq s390irq = {};
>
> if (copy_from_user(&s390int, argp, sizeof(s390int)))
> return -EFAULT;
>
> would certainly be ok as well, but
I don't think that it's urgently necessary, but ok, if you all prefer to
have it, too, I can add it to my patch.
>> But the more important question: Do we then still care of fixing the
>> PFAULT_INIT interrupt here? Since it requires a parameter, the "case
>> KVM_S390_INT_PFAULT_INIT:" part would be required here anyway.
>
> as long as we we this interface we should fix it and we should do the
> pfault thing correctly.
> Maybe we should start to deprecate this interface and remove it.
Hmm, we already talked about deprecating support for pre-3.15 kernel
stuff in the past (see
https://wiki.qemu.org/ChangeLog/2.12#Future_incompatible_changes for
example), since this has been broken in QEMU since quite a while, but
the new KVM_S390_IRQ replacement has just been introduced with kernel
4.1 ... so removing this KVM_S390_INTERRUPT ioctl any time soon sounds
wrong to me, we might break some userspace programs that are still there
in the wild...
Thomas
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-12 11:23 ` Thomas Huth
@ 2019-09-13 7:20 ` Cornelia Huck
2019-09-13 7:34 ` Thomas Huth
0 siblings, 1 reply; 15+ messages in thread
From: Cornelia Huck @ 2019-09-13 7:20 UTC (permalink / raw)
To: Thomas Huth
Cc: Christian Borntraeger, David Hildenbrand, Janosch Frank, kvm,
linux-s390, linux-kernel
On Thu, 12 Sep 2019 13:23:38 +0200
Thomas Huth <thuth@redhat.com> wrote:
> Hmm, we already talked about deprecating support for pre-3.15 kernel
> stuff in the past (see
> https://wiki.qemu.org/ChangeLog/2.12#Future_incompatible_changes for
> example),
Btw: did we ever do that? I don't quite recall what code we were
talking about...
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-13 7:20 ` Cornelia Huck
@ 2019-09-13 7:34 ` Thomas Huth
2019-09-13 7:37 ` David Hildenbrand
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Huth @ 2019-09-13 7:34 UTC (permalink / raw)
To: Cornelia Huck
Cc: Christian Borntraeger, David Hildenbrand, Janosch Frank, kvm,
linux-s390, linux-kernel
On 13/09/2019 09.20, Cornelia Huck wrote:
> On Thu, 12 Sep 2019 13:23:38 +0200
> Thomas Huth <thuth@redhat.com> wrote:
>
>> Hmm, we already talked about deprecating support for pre-3.15 kernel
>> stuff in the past (see
>> https://wiki.qemu.org/ChangeLog/2.12#Future_incompatible_changes for
>> example),
>
> Btw: did we ever do that? I don't quite recall what code we were
> talking about...
We never really did - but we also never fixed the issue: If you run the
current QEMU on a kernel before 3.15, it refuses to work due to the
missing in-kernel FLIC device:
Initialization of device s390-flic-kvm failed: KVM is missing capability
KVM_CAP_DEVICE_CTR
Since nobody really complained so far that running QEMU with KVM is
still required on a kernel < 3.15, I think we could make this also
"official" now and improve the error message a little bit, pointing the
user to a kernel >= 3.15.
Thomas
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-13 7:34 ` Thomas Huth
@ 2019-09-13 7:37 ` David Hildenbrand
2019-09-13 7:43 ` Thomas Huth
0 siblings, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2019-09-13 7:37 UTC (permalink / raw)
To: Thomas Huth, Cornelia Huck
Cc: Christian Borntraeger, Janosch Frank, kvm, linux-s390,
linux-kernel
On 13.09.19 09:34, Thomas Huth wrote:
> On 13/09/2019 09.20, Cornelia Huck wrote:
>> On Thu, 12 Sep 2019 13:23:38 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> Hmm, we already talked about deprecating support for pre-3.15 kernel
>>> stuff in the past (see
>>> https://wiki.qemu.org/ChangeLog/2.12#Future_incompatible_changes for
>>> example),
>>
>> Btw: did we ever do that? I don't quite recall what code we were
>> talking about...
>
> We never really did - but we also never fixed the issue: If you run the
> current QEMU on a kernel before 3.15, it refuses to work due to the
> missing in-kernel FLIC device:
>
> Initialization of device s390-flic-kvm failed: KVM is missing capability
> KVM_CAP_DEVICE_CTR
>
> Since nobody really complained so far that running QEMU with KVM is
> still required on a kernel < 3.15, I think we could make this also
> "official" now and improve the error message a little bit, pointing the
> user to a kernel >= 3.15.
Didn't we discuss back then to clean up *QEMU* and not the *kernel*?
Especially, to wait with cleanups until somebody requests to fix
instead. I mean you could have any user space in the wild that still
makes use of these interfaces ...
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-13 7:37 ` David Hildenbrand
@ 2019-09-13 7:43 ` Thomas Huth
2019-09-13 7:47 ` David Hildenbrand
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Huth @ 2019-09-13 7:43 UTC (permalink / raw)
To: David Hildenbrand, Cornelia Huck
Cc: Christian Borntraeger, Janosch Frank, kvm, linux-s390,
linux-kernel
On 13/09/2019 09.37, David Hildenbrand wrote:
> On 13.09.19 09:34, Thomas Huth wrote:
>> On 13/09/2019 09.20, Cornelia Huck wrote:
>>> On Thu, 12 Sep 2019 13:23:38 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> Hmm, we already talked about deprecating support for pre-3.15 kernel
>>>> stuff in the past (see
>>>> https://wiki.qemu.org/ChangeLog/2.12#Future_incompatible_changes for
>>>> example),
>>>
>>> Btw: did we ever do that? I don't quite recall what code we were
>>> talking about...
>>
>> We never really did - but we also never fixed the issue: If you run the
>> current QEMU on a kernel before 3.15, it refuses to work due to the
>> missing in-kernel FLIC device:
>>
>> Initialization of device s390-flic-kvm failed: KVM is missing capability
>> KVM_CAP_DEVICE_CTR
>>
>> Since nobody really complained so far that running QEMU with KVM is
>> still required on a kernel < 3.15, I think we could make this also
>> "official" now and improve the error message a little bit, pointing the
>> user to a kernel >= 3.15.
>
> Didn't we discuss back then to clean up *QEMU* and not the *kernel*?
> Especially, to wait with cleanups until somebody requests to fix
> instead. I mean you could have any user space in the wild that still
> makes use of these interfaces ...
Yes, that error message is part of QEMU, so I was referring to that one.
Sorry for mixing this into a mail thread on the kernel mailing list :-/
Thomas
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-13 7:43 ` Thomas Huth
@ 2019-09-13 7:47 ` David Hildenbrand
0 siblings, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-09-13 7:47 UTC (permalink / raw)
To: Thomas Huth, Cornelia Huck
Cc: Christian Borntraeger, Janosch Frank, kvm, linux-s390,
linux-kernel
On 13.09.19 09:43, Thomas Huth wrote:
> On 13/09/2019 09.37, David Hildenbrand wrote:
>> On 13.09.19 09:34, Thomas Huth wrote:
>>> On 13/09/2019 09.20, Cornelia Huck wrote:
>>>> On Thu, 12 Sep 2019 13:23:38 +0200
>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>>> Hmm, we already talked about deprecating support for pre-3.15 kernel
>>>>> stuff in the past (see
>>>>> https://wiki.qemu.org/ChangeLog/2.12#Future_incompatible_changes for
>>>>> example),
>>>>
>>>> Btw: did we ever do that? I don't quite recall what code we were
>>>> talking about...
>>>
>>> We never really did - but we also never fixed the issue: If you run the
>>> current QEMU on a kernel before 3.15, it refuses to work due to the
>>> missing in-kernel FLIC device:
>>>
>>> Initialization of device s390-flic-kvm failed: KVM is missing capability
>>> KVM_CAP_DEVICE_CTR
>>>
>>> Since nobody really complained so far that running QEMU with KVM is
>>> still required on a kernel < 3.15, I think we could make this also
>>> "official" now and improve the error message a little bit, pointing the
>>> user to a kernel >= 3.15.
>>
>> Didn't we discuss back then to clean up *QEMU* and not the *kernel*?
>> Especially, to wait with cleanups until somebody requests to fix
>> instead. I mean you could have any user space in the wild that still
>> makes use of these interfaces ...
>
> Yes, that error message is part of QEMU, so I was referring to that one.
> Sorry for mixing this into a mail thread on the kernel mailing list :-/
Ah okay, I messed up then :)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-12 9:20 ` Thomas Huth
2019-09-12 9:28 ` Janosch Frank
2019-09-12 10:52 ` Christian Borntraeger
@ 2019-09-12 10:58 ` David Hildenbrand
2019-09-12 11:00 ` Christian Borntraeger
2 siblings, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2019-09-12 10:58 UTC (permalink / raw)
To: Thomas Huth, Christian Borntraeger, Janosch Frank, kvm
Cc: Cornelia Huck, linux-s390, linux-kernel
On 12.09.19 11:20, Thomas Huth wrote:
> On 12/09/2019 11.14, David Hildenbrand wrote:
>> On 12.09.19 11:00, Thomas Huth wrote:
>>> When the userspace program runs the KVM_S390_INTERRUPT ioctl to inject
>>> an interrupt, we convert them from the legacy struct kvm_s390_interrupt
>>> to the new struct kvm_s390_irq via the s390int_to_s390irq() function.
>>> However, this function does not take care of all types of interrupts
>>> that we can inject into the guest later (see do_inject_vcpu()). Since we
>>> do not clear out the s390irq values before calling s390int_to_s390irq(),
>>> there is a chance that we copy unwanted data from the kernel stack
>>> into the guest memory later if the interrupt data has not been properly
>>> initialized by s390int_to_s390irq().
>>>
>>> Specifically, the problem exists with the KVM_S390_INT_PFAULT_INIT
>>> interrupt: s390int_to_s390irq() does not handle it, but the function
>>> __deliver_pfault_init() will later copy the uninitialized stack data
>>> from the ext.ext_params2 into the guest memory.
>>>
>>> Fix it by handling that interrupt type in s390int_to_s390irq(), too.
>>> And while we're at it, make sure that s390int_to_s390irq() now
>>> directly returns -EINVAL for unknown interrupt types, so that we
>>> do not run into this problem again in case we add more interrupt
>>> types to do_inject_vcpu() sometime in the future.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> arch/s390/kvm/interrupt.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index 3e7efdd9228a..165dea4c7f19 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -1960,6 +1960,16 @@ int s390int_to_s390irq(struct kvm_s390_interrupt *s390int,
>>> case KVM_S390_MCHK:
>>> irq->u.mchk.mcic = s390int->parm64;
>>> break;
>>> + case KVM_S390_INT_PFAULT_INIT:
>>> + irq->u.ext.ext_params = s390int->parm;
>>> + irq->u.ext.ext_params2 = s390int->parm64;
>>> + break;
>>> + case KVM_S390_RESTART:
>>> + case KVM_S390_INT_CLOCK_COMP:
>>> + case KVM_S390_INT_CPU_TIMER:
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> }
>>> return 0;
>>> }
>>>
>>
>> Wouldn't a safe fix be to initialize the struct to zero in the caller?
>
> That's of course possible, too. But that means that we always have to
> zero out the whole structure, so that's a little bit more of overhead
> (well, it likely doesn't matter for such a legacy ioctl).
I would vote for doing this as well.
>
> But the more important question: Do we then still care of fixing the
> PFAULT_INIT interrupt here? Since it requires a parameter, the "case
> KVM_S390_INT_PFAULT_INIT:" part would be required here anyway.
>
That's indeed true.
Reviewed-by: David Hildenbrand <david@redhat.com>
> Thomas
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: s390: Do not leak kernel stack data in the KVM_S390_INTERRUPT ioctl
2019-09-12 10:58 ` David Hildenbrand
@ 2019-09-12 11:00 ` Christian Borntraeger
0 siblings, 0 replies; 15+ messages in thread
From: Christian Borntraeger @ 2019-09-12 11:00 UTC (permalink / raw)
To: David Hildenbrand, Thomas Huth, Janosch Frank, kvm
Cc: Cornelia Huck, linux-s390, linux-kernel
On 12.09.19 12:58, David Hildenbrand wrote:
> On 12.09.19 11:20, Thomas Huth wrote:
>> On 12/09/2019 11.14, David Hildenbrand wrote:
>>> On 12.09.19 11:00, Thomas Huth wrote:
>>>> When the userspace program runs the KVM_S390_INTERRUPT ioctl to inject
>>>> an interrupt, we convert them from the legacy struct kvm_s390_interrupt
>>>> to the new struct kvm_s390_irq via the s390int_to_s390irq() function.
>>>> However, this function does not take care of all types of interrupts
>>>> that we can inject into the guest later (see do_inject_vcpu()). Since we
>>>> do not clear out the s390irq values before calling s390int_to_s390irq(),
>>>> there is a chance that we copy unwanted data from the kernel stack
>>>> into the guest memory later if the interrupt data has not been properly
>>>> initialized by s390int_to_s390irq().
>>>>
>>>> Specifically, the problem exists with the KVM_S390_INT_PFAULT_INIT
>>>> interrupt: s390int_to_s390irq() does not handle it, but the function
>>>> __deliver_pfault_init() will later copy the uninitialized stack data
>>>> from the ext.ext_params2 into the guest memory.
>>>>
>>>> Fix it by handling that interrupt type in s390int_to_s390irq(), too.
>>>> And while we're at it, make sure that s390int_to_s390irq() now
>>>> directly returns -EINVAL for unknown interrupt types, so that we
>>>> do not run into this problem again in case we add more interrupt
>>>> types to do_inject_vcpu() sometime in the future.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> arch/s390/kvm/interrupt.c | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>> index 3e7efdd9228a..165dea4c7f19 100644
>>>> --- a/arch/s390/kvm/interrupt.c
>>>> +++ b/arch/s390/kvm/interrupt.c
>>>> @@ -1960,6 +1960,16 @@ int s390int_to_s390irq(struct kvm_s390_interrupt *s390int,
>>>> case KVM_S390_MCHK:
>>>> irq->u.mchk.mcic = s390int->parm64;
>>>> break;
>>>> + case KVM_S390_INT_PFAULT_INIT:
>>>> + irq->u.ext.ext_params = s390int->parm;
>>>> + irq->u.ext.ext_params2 = s390int->parm64;
>>>> + break;
>>>> + case KVM_S390_RESTART:
>>>> + case KVM_S390_INT_CLOCK_COMP:
>>>> + case KVM_S390_INT_CPU_TIMER:
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>>> }
>>>> return 0;
>>>> }
>>>>
>>>
>>> Wouldn't a safe fix be to initialize the struct to zero in the caller?
>>
>> That's of course possible, too. But that means that we always have to
>> zero out the whole structure, so that's a little bit more of overhead
>> (well, it likely doesn't matter for such a legacy ioctl).
>
> I would vote for doing this as well.
Yes, lets also do the designated initializer, add more text to the patch
description (or should we not?) add cc stable and I will pick a v2.
>
>>
>> But the more important question: Do we then still care of fixing the
>> PFAULT_INIT interrupt here? Since it requires a parameter, the "case
>> KVM_S390_INT_PFAULT_INIT:" part would be required here anyway.
>>
>
> That's indeed true.
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
>> Thomas
>>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread