kvm: suppress KVM_SET_GSI_ROUTING allocation failure

Commit Message

Michal Hocko Feb. 13, 2018, 2:48 p.m. UTC

On Thu 08-02-18 13:35:08, David Rientjes wrote:
> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
> This can be up to 4096 entries on architectures such as arm64 and s390
> (and the upper bound may be increased on s390 eventually).
> 
> This can produce a vmalloc allocation failure warning:
> 
> vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)

I am not arguing about the kvm change but do we actaully want to warn
for 0 sized allocations? This just doesn't make much sense to me.
In other words don't we want this?

Comments

Paolo Bonzini Feb. 13, 2018, 3:03 p.m. UTC | #1

On 13/02/2018 15:48, Michal Hocko wrote:
> On Thu 08-02-18 13:35:08, David Rientjes wrote:
>> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
>> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
>> This can be up to 4096 entries on architectures such as arm64 and s390
>> (and the upper bound may be increased on s390 eventually).
>>
>> This can produce a vmalloc allocation failure warning:
>>
>> vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)
> 
> I am not arguing about the kvm change but do we actaully want to warn
> for 0 sized allocations? This just doesn't make much sense to me.
> In other words don't we want this?
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 673942094328..c5d832510c54 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1748,7 +1748,9 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
>  	unsigned long real_size = size;
>  
>  	size = PAGE_ALIGN(size);
> -	if (!size || (size >> PAGE_SHIFT) > totalram_pages)
> +	if (!size)
> +		return NULL;
> +	if ((size >> PAGE_SHIFT) > totalram_pages)
>  		goto fail;
>  
>  	area = __get_vm_area_node(size, align, VM_ALLOC | VM_UNINITIALIZED |
> 

There have been quite a few reports of this from syzkaller and generally
we've fixed them.  It does seem like a recipe for NULL-pointer
dereferences when the size is user-controlled (as in this case).

But here I'm actually not sure that the "allocation failure: 0 bytes"
can happen, since we have a check above for "if (routing.nr)", and there
is a check also so that the maximum allocation here is a meager 128 KiB.
 So I'm wondering if this patch is obsolete actually after commit
f8c1b85b2523.  David?

Thanks,

Paolo

Paolo

Michal Hocko Feb. 13, 2018, 3:44 p.m. UTC | #2

On Tue 13-02-18 16:03:09, Paolo Bonzini wrote:
> On 13/02/2018 15:48, Michal Hocko wrote:
> > On Thu 08-02-18 13:35:08, David Rientjes wrote:
> >> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
> >> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
> >> This can be up to 4096 entries on architectures such as arm64 and s390
> >> (and the upper bound may be increased on s390 eventually).
> >>
> >> This can produce a vmalloc allocation failure warning:
> >>
> >> vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)
> > 
> > I am not arguing about the kvm change but do we actaully want to warn
> > for 0 sized allocations? This just doesn't make much sense to me.
> > In other words don't we want this?
> > 
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 673942094328..c5d832510c54 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -1748,7 +1748,9 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> >  	unsigned long real_size = size;
> >  
> >  	size = PAGE_ALIGN(size);
> > -	if (!size || (size >> PAGE_SHIFT) > totalram_pages)
> > +	if (!size)
> > +		return NULL;
> > +	if ((size >> PAGE_SHIFT) > totalram_pages)
> >  		goto fail;
> >  
> >  	area = __get_vm_area_node(size, align, VM_ALLOC | VM_UNINITIALIZED |
> > 
> 
> There have been quite a few reports of this from syzkaller and generally
> we've fixed them.  It does seem like a recipe for NULL-pointer
> dereferences when the size is user-controlled (as in this case).

We do return NULL for that case regardless the above. The patch just
doesn't warn. Or do you think it is helpful to warn?

Paolo Bonzini Feb. 13, 2018, 3:49 p.m. UTC | #3

On 13/02/2018 16:44, Michal Hocko wrote:
> On Tue 13-02-18 16:03:09, Paolo Bonzini wrote:
>> On 13/02/2018 15:48, Michal Hocko wrote:
>>> On Thu 08-02-18 13:35:08, David Rientjes wrote:
>>>> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
>>>> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
>>>> This can be up to 4096 entries on architectures such as arm64 and s390
>>>> (and the upper bound may be increased on s390 eventually).
>>>>
>>>> This can produce a vmalloc allocation failure warning:
>>>>
>>>> vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)
>>>
>>> I am not arguing about the kvm change but do we actaully want to warn
>>> for 0 sized allocations? This just doesn't make much sense to me.
>>> In other words don't we want this?
>>>
>>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>>> index 673942094328..c5d832510c54 100644
>>> --- a/mm/vmalloc.c
>>> +++ b/mm/vmalloc.c
>>> @@ -1748,7 +1748,9 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
>>>  	unsigned long real_size = size;
>>>  
>>>  	size = PAGE_ALIGN(size);
>>> -	if (!size || (size >> PAGE_SHIFT) > totalram_pages)
>>> +	if (!size)
>>> +		return NULL;
>>> +	if ((size >> PAGE_SHIFT) > totalram_pages)
>>>  		goto fail;
>>>  
>>>  	area = __get_vm_area_node(size, align, VM_ALLOC | VM_UNINITIALIZED |
>>>
>>
>> There have been quite a few reports of this from syzkaller and generally
>> we've fixed them.  It does seem like a recipe for NULL-pointer
>> dereferences when the size is user-controlled (as in this case).
> 
> We do return NULL for that case regardless the above. The patch just
> doesn't warn. Or do you think it is helpful to warn?

It certainly helps bringing potential issues in the spotlight (through
fuzzing, mostly).

Paolo

Michal Hocko Feb. 13, 2018, 3:58 p.m. UTC | #4

On Tue 13-02-18 16:49:20, Paolo Bonzini wrote:
> On 13/02/2018 16:44, Michal Hocko wrote:
> > On Tue 13-02-18 16:03:09, Paolo Bonzini wrote:
[...]
> >> There have been quite a few reports of this from syzkaller and generally
> >> we've fixed them.  It does seem like a recipe for NULL-pointer
> >> dereferences when the size is user-controlled (as in this case).
> > 
> > We do return NULL for that case regardless the above. The patch just
> > doesn't warn. Or do you think it is helpful to warn?
> 
> It certainly helps bringing potential issues in the spotlight (through
> fuzzing, mostly).

Fair enough.

Patch

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 673942094328..c5d832510c54 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1748,7 +1748,9 @@  void *__vmalloc_node_range(unsigned long size, unsigned long align,
 	unsigned long real_size = size;
 
 	size = PAGE_ALIGN(size);
-	if (!size || (size >> PAGE_SHIFT) > totalram_pages)
+	if (!size)
+		return NULL;
+	if ((size >> PAGE_SHIFT) > totalram_pages)
 		goto fail;
 
 	area = __get_vm_area_node(size, align, VM_ALLOC | VM_UNINITIALIZED |