[PATCH v2 1/3] KVM: arm64: vgic-its: Handle errors from vgic_add_lpi

[Andre Przywara]

Hi,

On 03/08/16 17:13, Christoffer Dall wrote:
> During low memory conditions, we could be dereferencing a NULL pointer
> when vgic_add_lpi fails to allocate memory.
> 
> Consider for example this call sequence:
> 
>   vgic_its_cmd_handle_mapi
>       itte->irq = vgic_add_lpi(kvm, lpi_nr);

Ouch! Thanks for catching this unhandled error return!

>           update_lpi_config(kvm, itte->irq, NULL);
>               ret = kvm_read_guest(kvm, propbase + irq->intid
> 	                                             ^^^^
> 						     kaboom?
> 
> Instead, return an error pointer from vgic_add_lpi and check the return
> value from its single caller.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall at linaro.org>
> ---
> Changes since v1:
>  - Don't errornously get an extra kref refernce for the struct vgic_irq
>  - Don't rework the entire error handling of the function, but follow
>    what Marc suggested he prefers based on his fixup patch.
>  virt/kvm/arm/vgic/vgic-its.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 07411cf..424f7a5 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -51,7 +51,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid)
>  
>  	irq = kzalloc(sizeof(struct vgic_irq), GFP_KERNEL);
>  	if (!irq)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	INIT_LIST_HEAD(&irq->lpi_list);
>  	INIT_LIST_HEAD(&irq->ap_list);
> @@ -693,10 +693,11 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  	u32 device_id = its_cmd_get_deviceid(its_cmd);
>  	u32 event_id = its_cmd_get_id(its_cmd);
>  	u32 coll_id = its_cmd_get_collection(its_cmd);
> -	struct its_itte *itte;
> +	struct its_itte *itte, *new_itte = NULL;
>  	struct its_device *device;
>  	struct its_collection *collection, *new_coll = NULL;
>  	int lpi_nr;
> +	struct vgic_irq *irq;
>  
>  	device = find_its_device(its, device_id);
>  	if (!device)
> @@ -720,7 +721,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  
>  	itte = find_itte(its, device_id, event_id);
>  	if (!itte) {
> -		itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
> +		new_itte = itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);

Nit: Aren't double assignments frowned upon in the kernel?

>  		if (!itte) {
>  			if (new_coll)
>  				vgic_its_free_collection(its, coll_id);
> @@ -733,7 +734,16 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  
>  	itte->collection = collection;
>  	itte->lpi = lpi_nr;
> -	itte->irq = vgic_add_lpi(kvm, lpi_nr);
> +
> +	irq = vgic_add_lpi(kvm, lpi_nr);
> +	if (IS_ERR(irq)) {
> +		if (new_coll)
> +			vgic_its_free_collection(its, coll_id);
> +		kfree(new_itte);

But at this point we already have added that ITTE to the
device->itt_head, haven't we?
Since we hold the its_lock, would a simple:

		if (new_itte) {
			list_del(&itte->itte_list);
			kfree(new_itte);
		}

suffice to fix this?

> +		return PTR_ERR(irq);
> +	}
> +	itte->irq = irq;
> +
>  	update_affinity_itte(kvm, itte);
>  
>  	/*
> 

Cheers,
Andre.