| 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
| ||
|
Message-ID: <20121206024101.GB7175@local>
Date: Thu, 6 Dec 2012 03:41:01 +0100
From: "Hans J. Koch" <hjk@...sjkoch.de>
To: Vitalii Demianets <vitas@...factor.kiev.ua>
Cc: "Hans J. Koch" <hjk@...sjkoch.de>, Cong Ding <dinggnu@...il.com>,
linux-kernel@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing
issues
On Wed, Dec 05, 2012 at 11:22:57AM +0200, Vitalii Demianets wrote:
> 1. uioinfo was kfreed based on the presence of pdev->dev.of_node, which was
> obviously wrong and unrelated to the fact if uioinfo was allocated statically
> or dynamically. This patch introduces new flag which clearly shows if uioinfo
> was allocated dynamically and kfrees uioinfo based on that flag;
> 2. Fix: priv data was not freed in case platform_get_irq() failed. As it was
> caused mainly by improper exit labels naming, labels were renamed too;
> 3. The case of uioinfo AND pdev->dev.of_node both NULL (not initialized
> in platform data) was not treated properly.
I noticed two more issues. See below.
>
> Signed-off-by: Vitalii Demianets <vitas@...factor.kiev.ua>
>
> ---
> v2: Constants naming style
>
> ---
> drivers/uio/uio_pdrv_genirq.c | 44 ++++++++++++++++++++++++----------------
> 1 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 42202cd..75aaeee 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -30,6 +30,11 @@
>
> #define DRIVER_NAME "uio_pdrv_genirq"
>
> +enum {
> + UIO_IRQ_DISABLED = 0,
> + UIO_INFO_ALLOCED = 1,
> +};
> +
> struct uio_pdrv_genirq_platdata {
> struct uio_info *uioinfo;
> spinlock_t lock;
> @@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
> * remember the state so we can allow user space to enable it later.
> */
>
> - if (!test_and_set_bit(0, &priv->flags))
> + if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
> disable_irq_nosync(irq);
That is not safe. That flag can only be changed by userspace, and if the
userspace part is not running, the CPU on which the irq is pending will hang.
The handler has to disable the interrupt in any case.
(The original version had the same problem, I know...)
>
> return IRQ_HANDLED;
> @@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>
> spin_lock_irqsave(&priv->lock, flags);
> if (irq_on) {
> - if (test_and_clear_bit(0, &priv->flags))
> + if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
> enable_irq(dev_info->irq);
> } else {
> - if (!test_and_set_bit(0, &priv->flags))
> + if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
> disable_irq(dev_info->irq);
> }
> spin_unlock_irqrestore(&priv->lock, flags);
> @@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> struct uio_mem *uiomem;
> int ret = -EINVAL;
> int i;
> + bool uioinfo_alloced = false;
>
> - if (!uioinfo) {
> + if (!uioinfo && pdev->dev.of_node) {
> int irq;
>
> /* alloc uioinfo for one device */
> @@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> if (!uioinfo) {
> ret = -ENOMEM;
> dev_err(&pdev->dev, "unable to kmalloc\n");
> - goto bad2;
> + goto out;
> }
> uioinfo->name = pdev->dev.of_node->name;
> uioinfo->version = "devicetree";
> + uioinfo_alloced = true;
>
> /* Multiple IRQs are not supported */
> irq = platform_get_irq(pdev, 0);
> @@ -125,32 +132,33 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>
> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> dev_err(&pdev->dev, "missing platform_data\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> if (uioinfo->handler || uioinfo->irqcontrol ||
> uioinfo->irq_flags & IRQF_SHARED) {
> dev_err(&pdev->dev, "interrupt configuration error\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> if (!priv) {
> ret = -ENOMEM;
> dev_err(&pdev->dev, "unable to kmalloc\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv->uioinfo = uioinfo;
> spin_lock_init(&priv->lock);
> - priv->flags = 0; /* interrupt is enabled to begin with */
> + /* interrupt is enabled to begin with */
> + priv->flags = uioinfo_alloced ? (1 << UIO_INFO_ALLOCED) : 0;
The comment doesn't describe the line it's supposed to comment.
> priv->pdev = pdev;
>
> if (!uioinfo->irq) {
> ret = platform_get_irq(pdev, 0);
> if (ret < 0) {
> dev_err(&pdev->dev, "failed to get IRQ\n");
> - goto bad0;
> + goto out_priv;
> }
> uioinfo->irq = ret;
> }
> @@ -205,19 +213,19 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> ret = uio_register_device(&pdev->dev, priv->uioinfo);
> if (ret) {
> dev_err(&pdev->dev, "unable to register uio device\n");
> - goto bad1;
> + goto out_pm;
> }
>
> platform_set_drvdata(pdev, priv);
> return 0;
> - bad1:
> - kfree(priv);
> +out_pm:
> pm_runtime_disable(&pdev->dev);
> - bad0:
> - /* kfree uioinfo for OF */
> - if (pdev->dev.of_node)
> +out_priv:
> + kfree(priv);
> +out_uioinfo:
> + if (uioinfo_alloced)
> kfree(uioinfo);
> - bad2:
> +out:
> return ret;
> }
>
> @@ -232,7 +240,7 @@ static int uio_pdrv_genirq_remove(struct platform_device *pdev)
> priv->uioinfo->irqcontrol = NULL;
>
> /* kfree uioinfo for OF */
> - if (pdev->dev.of_node)
> + if (priv->flags & (1 << UIO_INFO_ALLOCED))
> kfree(priv->uioinfo);
>
> kfree(priv);
> --
> 1.7.8.6
>
--
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