lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Fri, 21 Mar 2014 08:56:14 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Alexey Khoroshilov <khoroshilov@...ras.ru>
CC:	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	Hans Verkuil <hans.verkuil@...co.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	ldv-project@...uxtesting.org
Subject: Re: [PATCH] [media] adv7180: free an interrupt on failure paths in
 init_device()

On 03/14/2014 10:04 PM, Alexey Khoroshilov wrote:
> There is request_irq() in init_device(), but the interrupt is not removed
> on failure paths. The patch adds proper error handling.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@...ras.ru>

Hi,

Thanks for the patch. It's actually worse than that. request_irq should not 
be called in init_device() since init_device() is also called on resume(). 
The request_irq call should be moved to probe() and be called after init 
device. I've recently made some modifications to this part of the driver 
(switched to threaded IRQs), so make sure you generate the patch against the 
media/master tree.

Thanks,
- Lars

> ---
>   drivers/media/i2c/adv7180.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index d7d99f1c69e4..e462392ba043 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -541,40 +541,44 @@ static int init_device(struct i2c_client *client, struct adv7180_state *state)
>   		ret = i2c_smbus_write_byte_data(client, ADV7180_ADI_CTRL_REG,
>   						ADV7180_ADI_CTRL_IRQ_SPACE);
>   		if (ret < 0)
> -			return ret;
> +			goto err;
>
>   		/* config the Interrupt pin to be active low */
>   		ret = i2c_smbus_write_byte_data(client, ADV7180_ICONF1_ADI,
>   						ADV7180_ICONF1_ACTIVE_LOW |
>   						ADV7180_ICONF1_PSYNC_ONLY);
>   		if (ret < 0)
> -			return ret;
> +			goto err;
>
>   		ret = i2c_smbus_write_byte_data(client, ADV7180_IMR1_ADI, 0);
>   		if (ret < 0)
> -			return ret;
> +			goto err;
>
>   		ret = i2c_smbus_write_byte_data(client, ADV7180_IMR2_ADI, 0);
>   		if (ret < 0)
> -			return ret;
> +			goto err;
>
>   		/* enable AD change interrupts interrupts */
>   		ret = i2c_smbus_write_byte_data(client, ADV7180_IMR3_ADI,
>   						ADV7180_IRQ3_AD_CHANGE);
>   		if (ret < 0)
> -			return ret;
> +			goto err;
>
>   		ret = i2c_smbus_write_byte_data(client, ADV7180_IMR4_ADI, 0);
>   		if (ret < 0)
> -			return ret;
> +			goto err;
>
>   		ret = i2c_smbus_write_byte_data(client, ADV7180_ADI_CTRL_REG,
>   						0);
>   		if (ret < 0)
> -			return ret;
> +			goto err;
>   	}
>
>   	return 0;
> +
> +err:
> +	free_irq(state->irq, state);
> +	return ret;
>   }
>
>   static int adv7180_probe(struct i2c_client *client,
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists