* [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()
@ 2019-09-09 14:55 Igor Mammedov
2019-09-09 15:47 ` David Hildenbrand
2019-09-09 16:21 ` Christian Borntraeger
0 siblings, 2 replies; 6+ messages in thread
From: Igor Mammedov @ 2019-09-09 14:55 UTC (permalink / raw)
To: linux-kernel; +Cc: borntraeger, david, cohuck
If userspace doesn't set KVM_MEM_LOG_DIRTY_PAGES on memslot before calling
kvm_s390_vm_start_migration(), kernel will oops with:
Unable to handle kernel pointer dereference in virtual kernel address space
Failing address: 0000000000000000 TEID: 0000000000000483
Fault in home space mode while using kernel ASCE.
AS:0000000002a2000b R2:00000001bff8c00b R3:00000001bff88007 S:00000001bff91000 P:000000000000003d
Oops: 0004 ilc:2 [#1] SMP
...
Call Trace:
([<001fffff804ec552>] kvm_s390_vm_set_attr+0x347a/0x3828 [kvm])
[<001fffff804ecfc0>] kvm_arch_vm_ioctl+0x6c0/0x1998 [kvm]
[<001fffff804b67e4>] kvm_vm_ioctl+0x51c/0x11a8 [kvm]
[<00000000008ba572>] do_vfs_ioctl+0x1d2/0xe58
[<00000000008bb284>] ksys_ioctl+0x8c/0xb8
[<00000000008bb2e2>] sys_ioctl+0x32/0x40
[<000000000175552c>] system_call+0x2b8/0x2d8
INFO: lockdep is turned off.
Last Breaking-Event-Address:
[<0000000000dbaf60>] __memset+0xc/0xa0
due to ms->dirty_bitmap being NULL, which migh crash the host.
Make sure that ms->dirty_bitmap is set before using it or
print a warning and return -ENIVAL otherwise.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS:
keeping it private for now as issue might DoS host,
I'll leave it upto maintainers to decide if it should be handled as security
bug (I'm not sure what process for handling such bugs should be used).
arch/s390/kvm/kvm-s390.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index f329dcb3f44c..dfba51c9d60c 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1018,6 +1018,10 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
/* mark all the pages in active slots as dirty */
for (slotnr = 0; slotnr < slots->used_slots; slotnr++) {
ms = slots->memslots + slotnr;
+ if (!ms->dirty_bitmap) {
+ WARN(1, "ms->dirty_bitmap == NULL\n");
+ return -EINVAL;
+ }
/*
* The second half of the bitmap is only used on x86,
* and would be wasted otherwise, so we put it to good
--
2.18.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()
2019-09-09 14:55 [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset() Igor Mammedov
@ 2019-09-09 15:47 ` David Hildenbrand
2019-09-09 16:17 ` Igor Mammedov
2019-09-09 16:21 ` Christian Borntraeger
1 sibling, 1 reply; 6+ messages in thread
From: David Hildenbrand @ 2019-09-09 15:47 UTC (permalink / raw)
To: Igor Mammedov, linux-kernel; +Cc: borntraeger, cohuck
On 09.09.19 16:55, Igor Mammedov wrote:
> If userspace doesn't set KVM_MEM_LOG_DIRTY_PAGES on memslot before calling
> kvm_s390_vm_start_migration(), kernel will oops with:
>
We usually have the subject in a "KVM: s390: ..." format. Like
"KVM: s390: check dirty_bitmap before using it as target for memset()"
> Unable to handle kernel pointer dereference in virtual kernel address space
> Failing address: 0000000000000000 TEID: 0000000000000483
> Fault in home space mode while using kernel ASCE.
> AS:0000000002a2000b R2:00000001bff8c00b R3:00000001bff88007 S:00000001bff91000 P:000000000000003d
> Oops: 0004 ilc:2 [#1] SMP
> ...
> Call Trace:
> ([<001fffff804ec552>] kvm_s390_vm_set_attr+0x347a/0x3828 [kvm])
> [<001fffff804ecfc0>] kvm_arch_vm_ioctl+0x6c0/0x1998 [kvm]
> [<001fffff804b67e4>] kvm_vm_ioctl+0x51c/0x11a8 [kvm]
> [<00000000008ba572>] do_vfs_ioctl+0x1d2/0xe58
> [<00000000008bb284>] ksys_ioctl+0x8c/0xb8
> [<00000000008bb2e2>] sys_ioctl+0x32/0x40
> [<000000000175552c>] system_call+0x2b8/0x2d8
> INFO: lockdep is turned off.
> Last Breaking-Event-Address:
> [<0000000000dbaf60>] __memset+0xc/0xa0
>
> due to ms->dirty_bitmap being NULL, which migh crash the host.
s/migh/might/
>
> Make sure that ms->dirty_bitmap is set before using it or
> print a warning and return -ENIVAL otherwise.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>
> PS:
> keeping it private for now as issue might DoS host,
> I'll leave it upto maintainers to decide if it should be handled as security
> bug (I'm not sure what process for handling such bugs should be used).
>
>
> arch/s390/kvm/kvm-s390.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index f329dcb3f44c..dfba51c9d60c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1018,6 +1018,10 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
> /* mark all the pages in active slots as dirty */
> for (slotnr = 0; slotnr < slots->used_slots; slotnr++) {
> ms = slots->memslots + slotnr;
> + if (!ms->dirty_bitmap) {
> + WARN(1, "ms->dirty_bitmap == NULL\n");
> + return -EINVAL;
> + }
if (WARN_ON_ONCE(!ms->dirty_bitmap))
return -EINVAL;
But I wonder if the WARN is really needed. (or simply a wrong usage of the interface)
> /*
> * The second half of the bitmap is only used on x86,
> * and would be wasted otherwise, so we put it to good
>
You should add
Fixes: afdad61615cc ("KVM: s390: Fix storage attributes migration with memory slots")
Cc: stable@vger.kernel.org # v4.19+
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()
2019-09-09 15:47 ` David Hildenbrand
@ 2019-09-09 16:17 ` Igor Mammedov
2019-09-09 16:22 ` David Hildenbrand
0 siblings, 1 reply; 6+ messages in thread
From: Igor Mammedov @ 2019-09-09 16:17 UTC (permalink / raw)
To: David Hildenbrand; +Cc: linux-kernel, borntraeger, cohuck
On Mon, 9 Sep 2019 17:47:37 +0200
David Hildenbrand <david@redhat.com> wrote:
> On 09.09.19 16:55, Igor Mammedov wrote:
> > If userspace doesn't set KVM_MEM_LOG_DIRTY_PAGES on memslot before calling
> > kvm_s390_vm_start_migration(), kernel will oops with:
> >
>
> We usually have the subject in a "KVM: s390: ..." format. Like
>
> "KVM: s390: check dirty_bitmap before using it as target for memset()"
>
> > Unable to handle kernel pointer dereference in virtual kernel address space
> > Failing address: 0000000000000000 TEID: 0000000000000483
> > Fault in home space mode while using kernel ASCE.
> > AS:0000000002a2000b R2:00000001bff8c00b R3:00000001bff88007 S:00000001bff91000 P:000000000000003d
> > Oops: 0004 ilc:2 [#1] SMP
> > ...
> > Call Trace:
> > ([<001fffff804ec552>] kvm_s390_vm_set_attr+0x347a/0x3828 [kvm])
> > [<001fffff804ecfc0>] kvm_arch_vm_ioctl+0x6c0/0x1998 [kvm]
> > [<001fffff804b67e4>] kvm_vm_ioctl+0x51c/0x11a8 [kvm]
> > [<00000000008ba572>] do_vfs_ioctl+0x1d2/0xe58
> > [<00000000008bb284>] ksys_ioctl+0x8c/0xb8
> > [<00000000008bb2e2>] sys_ioctl+0x32/0x40
> > [<000000000175552c>] system_call+0x2b8/0x2d8
> > INFO: lockdep is turned off.
> > Last Breaking-Event-Address:
> > [<0000000000dbaf60>] __memset+0xc/0xa0
> >
> > due to ms->dirty_bitmap being NULL, which migh crash the host.
>
> s/migh/might/
>
> >
> > Make sure that ms->dirty_bitmap is set before using it or
> > print a warning and return -ENIVAL otherwise.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >
> > PS:
> > keeping it private for now as issue might DoS host,
> > I'll leave it upto maintainers to decide if it should be handled as security
> > bug (I'm not sure what process for handling such bugs should be used).
> >
> >
> > arch/s390/kvm/kvm-s390.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index f329dcb3f44c..dfba51c9d60c 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -1018,6 +1018,10 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
> > /* mark all the pages in active slots as dirty */
> > for (slotnr = 0; slotnr < slots->used_slots; slotnr++) {
> > ms = slots->memslots + slotnr;
> > + if (!ms->dirty_bitmap) {
> > + WARN(1, "ms->dirty_bitmap == NULL\n");
> > + return -EINVAL;
> > + }
>
> if (WARN_ON_ONCE(!ms->dirty_bitmap))
> return -EINVAL;
>
> But I wonder if the WARN is really needed. (or simply a wrong usage of the interface)
I added WARN because of there is no any visible sign that something
went wrong in userspace, this way at least we would have a trace of
invalid API usage.
But I can drop it if you prefer.
>
>
> > /*
> > * The second half of the bitmap is only used on x86,
> > * and would be wasted otherwise, so we put it to good
> >
>
> You should add
>
> Fixes: afdad61615cc ("KVM: s390: Fix storage attributes migration with memory slots")
> Cc: stable@vger.kernel.org # v4.19+
>
> Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()
2019-09-09 14:55 [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset() Igor Mammedov
2019-09-09 15:47 ` David Hildenbrand
@ 2019-09-09 16:21 ` Christian Borntraeger
2019-09-10 10:21 ` Claudio Imbrenda
1 sibling, 1 reply; 6+ messages in thread
From: Christian Borntraeger @ 2019-09-09 16:21 UTC (permalink / raw)
To: Igor Mammedov, linux-kernel
Cc: david, cohuck, Janosch Frank, Claudio Imbrenda
Adding Janosch (co-maintainer) and Claudio (author of that code)
On 09.09.19 16:55, Igor Mammedov wrote:
> If userspace doesn't set KVM_MEM_LOG_DIRTY_PAGES on memslot before calling
> kvm_s390_vm_start_migration(), kernel will oops with:
>
> Unable to handle kernel pointer dereference in virtual kernel address space
> Failing address: 0000000000000000 TEID: 0000000000000483
> Fault in home space mode while using kernel ASCE.
> AS:0000000002a2000b R2:00000001bff8c00b R3:00000001bff88007 S:00000001bff91000 P:000000000000003d
> Oops: 0004 ilc:2 [#1] SMP
> ...
> Call Trace:
> ([<001fffff804ec552>] kvm_s390_vm_set_attr+0x347a/0x3828 [kvm])
> [<001fffff804ecfc0>] kvm_arch_vm_ioctl+0x6c0/0x1998 [kvm]
> [<001fffff804b67e4>] kvm_vm_ioctl+0x51c/0x11a8 [kvm]
> [<00000000008ba572>] do_vfs_ioctl+0x1d2/0xe58
> [<00000000008bb284>] ksys_ioctl+0x8c/0xb8
> [<00000000008bb2e2>] sys_ioctl+0x32/0x40
> [<000000000175552c>] system_call+0x2b8/0x2d8
> INFO: lockdep is turned off.
> Last Breaking-Event-Address:
> [<0000000000dbaf60>] __memset+0xc/0xa0
>
> due to ms->dirty_bitmap being NULL, which migh crash the host.
>
> Make sure that ms->dirty_bitmap is set before using it or
> print a warning and return -ENIVAL otherwise.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>
> PS:
> keeping it private for now as issue might DoS host,
> I'll leave it upto maintainers to decide if it should be handled as security
> bug (I'm not sure what process for handling such bugs should be used).
I think its fine to send to the public lists. Its just a bug and with cc stable this
should cascade quickly to the right places.
>
>
> arch/s390/kvm/kvm-s390.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index f329dcb3f44c..dfba51c9d60c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1018,6 +1018,10 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
> /* mark all the pages in active slots as dirty */
> for (slotnr = 0; slotnr < slots->used_slots; slotnr++) {
> ms = slots->memslots + slotnr;
> + if (!ms->dirty_bitmap) {
> + WARN(1, "ms->dirty_bitmap == NULL\n");
I would prefer to not have a WARN_ON. Otherwise this would allow a malicious user
to spam the log.
> + return -EINVAL;
> + }
> /*
> * The second half of the bitmap is only used on x86,
> * and would be wasted otherwise, so we put it to good
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()
2019-09-09 16:17 ` Igor Mammedov
@ 2019-09-09 16:22 ` David Hildenbrand
0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2019-09-09 16:22 UTC (permalink / raw)
To: Igor Mammedov; +Cc: linux-kernel, borntraeger, cohuck
On 09.09.19 18:17, Igor Mammedov wrote:
> On Mon, 9 Sep 2019 17:47:37 +0200
> David Hildenbrand <david@redhat.com> wrote:
>
>> On 09.09.19 16:55, Igor Mammedov wrote:
>>> If userspace doesn't set KVM_MEM_LOG_DIRTY_PAGES on memslot before calling
>>> kvm_s390_vm_start_migration(), kernel will oops with:
>>>
>>
>> We usually have the subject in a "KVM: s390: ..." format. Like
>>
>> "KVM: s390: check dirty_bitmap before using it as target for memset()"
>>
>>> Unable to handle kernel pointer dereference in virtual kernel address space
>>> Failing address: 0000000000000000 TEID: 0000000000000483
>>> Fault in home space mode while using kernel ASCE.
>>> AS:0000000002a2000b R2:00000001bff8c00b R3:00000001bff88007 S:00000001bff91000 P:000000000000003d
>>> Oops: 0004 ilc:2 [#1] SMP
>>> ...
>>> Call Trace:
>>> ([<001fffff804ec552>] kvm_s390_vm_set_attr+0x347a/0x3828 [kvm])
>>> [<001fffff804ecfc0>] kvm_arch_vm_ioctl+0x6c0/0x1998 [kvm]
>>> [<001fffff804b67e4>] kvm_vm_ioctl+0x51c/0x11a8 [kvm]
>>> [<00000000008ba572>] do_vfs_ioctl+0x1d2/0xe58
>>> [<00000000008bb284>] ksys_ioctl+0x8c/0xb8
>>> [<00000000008bb2e2>] sys_ioctl+0x32/0x40
>>> [<000000000175552c>] system_call+0x2b8/0x2d8
>>> INFO: lockdep is turned off.
>>> Last Breaking-Event-Address:
>>> [<0000000000dbaf60>] __memset+0xc/0xa0
>>>
>>> due to ms->dirty_bitmap being NULL, which migh crash the host.
>>
>> s/migh/might/
>>
>>>
>>> Make sure that ms->dirty_bitmap is set before using it or
>>> print a warning and return -ENIVAL otherwise.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>>
>>> PS:
>>> keeping it private for now as issue might DoS host,
>>> I'll leave it upto maintainers to decide if it should be handled as security
>>> bug (I'm not sure what process for handling such bugs should be used).
>>>
>>>
>>> arch/s390/kvm/kvm-s390.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index f329dcb3f44c..dfba51c9d60c 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -1018,6 +1018,10 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
>>> /* mark all the pages in active slots as dirty */
>>> for (slotnr = 0; slotnr < slots->used_slots; slotnr++) {
>>> ms = slots->memslots + slotnr;
>>> + if (!ms->dirty_bitmap) {
>>> + WARN(1, "ms->dirty_bitmap == NULL\n");
>>> + return -EINVAL;
>>> + }
>>
>> if (WARN_ON_ONCE(!ms->dirty_bitmap))
>> return -EINVAL;
>>
>> But I wonder if the WARN is really needed. (or simply a wrong usage of the interface)
> I added WARN because of there is no any visible sign that something
> went wrong in userspace, this way at least we would have a trace of
> invalid API usage.
>
> But I can drop it if you prefer.
Yeah, I understand the rational, but WARN's should usually not be
triggered from user space (and -EINVAL is some telling user space
already that it is doing something wrong). If you care about a
notification maybe use a pr_warn_ratelimited() instead. Otherwise, I
suggest updating the documentation in which cases one could get -EINVAL.
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()
2019-09-09 16:21 ` Christian Borntraeger
@ 2019-09-10 10:21 ` Claudio Imbrenda
0 siblings, 0 replies; 6+ messages in thread
From: Claudio Imbrenda @ 2019-09-10 10:21 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Igor Mammedov, linux-kernel, david, cohuck, Janosch Frank
On Mon, 9 Sep 2019 18:21:47 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
[...]
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index f329dcb3f44c..dfba51c9d60c 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -1018,6 +1018,10 @@ static int
> > kvm_s390_vm_start_migration(struct kvm *kvm) /* mark all the pages
> > in active slots as dirty */ for (slotnr = 0; slotnr <
> > slots->used_slots; slotnr++) { ms = slots->memslots + slotnr;
> > + if (!ms->dirty_bitmap) {
> > + WARN(1, "ms->dirty_bitmap == NULL\n");
>
> I would prefer to not have a WARN_ON. Otherwise this would allow a
> malicious user to spam the log.
I agree, the WARN is not needed.
>
> > + return -EINVAL;
> > + }
> > /*
> > * The second half of the bitmap is only used on
> > x86,
> > * and would be wasted otherwise, so we put it to
> > good
Otherwise it looks good.
Claudio Imbrenda
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-10 10:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-09 14:55 [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset() Igor Mammedov
2019-09-09 15:47 ` David Hildenbrand
2019-09-09 16:17 ` Igor Mammedov
2019-09-09 16:22 ` David Hildenbrand
2019-09-09 16:21 ` Christian Borntraeger
2019-09-10 10:21 ` Claudio Imbrenda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).