Re: [PATCH] usb: misc: ljca: Fix double free in error handling path

[Hans de Goede]

Hi,

On 3/6/24 14:00, Yongzhi Liu wrote:
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), callback function ljca_auxdev_release
> calls kfree(auxdev->dev.platform_data) to free the parameter data
> of the function ljca_new_client_device. The callers of
> ljca_new_client_device shouldn't call kfree() again
> in the error handling path to free the platform data.
> 
> Fix this by cleaning up the redundant kfree().

If things fail in ljca_new_client_device() before 
auxiliary_device_init() gets called (or if that call fails)
then auxiliary_device_uninit() will NOT be called leading to
a memory-leak.

So this patch is no good as is.

To properly fix this you must make ljca_new_client_device() 
always take ownership of the passed in platform_data and
make it also kfree() the passed in platform_data on errors
which happen before auxiliary_device_init() succeeds.

Regards,

Hans





> 
> Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
> Signed-off-by: Yongzhi Liu <hyperlyzcs@xxxxxxxxx>
> ---
>  drivers/usb/misc/usb-ljca.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> index 35770e608c64..be702364be08 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -590,12 +590,8 @@ static int ljca_enumerate_gpio(struct ljca_adapter *adap)
>  		valid_pin[i] = get_unaligned_le32(&desc->bank_desc[i].valid_pins);
>  	bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num);
>  
> -	ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
> +	return ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
>  				     gpio_info, LJCA_GPIO_ACPI_ADR);
> -	if (ret)
> -		kfree(gpio_info);
> -
> -	return ret;
>  }
>  
>  static int ljca_enumerate_i2c(struct ljca_adapter *adap)
> @@ -626,13 +622,9 @@ static int ljca_enumerate_i2c(struct ljca_adapter *adap)
>  		i2c_info->capacity = desc->info[i].capacity;
>  		i2c_info->intr_pin = desc->info[i].intr_pin;
>  
> -		ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
> +		return ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
>  					     "ljca-i2c", i2c_info,
>  					     LJCA_I2C1_ACPI_ADR + i);
> -		if (ret) {
> -			kfree(i2c_info);
> -			return ret;
> -		}
>  	}
>  
>  	return 0;
> @@ -666,13 +658,9 @@ static int ljca_enumerate_spi(struct ljca_adapter *adap)
>  		spi_info->id = desc->info[i].id;
>  		spi_info->capacity = desc->info[i].capacity;
>  
> -		ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
> +		return ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
>  					     "ljca-spi", spi_info,
>  					     LJCA_SPI1_ACPI_ADR + i);
> -		if (ret) {
> -			kfree(spi_info);
> -			return ret;
> -		}
>  	}
>  
>  	return 0;