[PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()

[PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()

[Igor Mammedov]

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

Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()

[David Hildenbrand]

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

Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()

[Igor Mammedov]

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!

Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()

[David Hildenbrand]

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

Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()

[Christian Borntraeger]

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
> 

Re: [PATCH] kvm_s390_vm_start_migration: check dirty_bitmap before using it as target for memset()

[Claudio Imbrenda]

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