linux-kernel - Re: [PATCH linux-next] agp: correct missing cleanup on error in agp_add

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20090728151918.9b1a793e.akpm@linux-foundation.org>
Date:	Tue, 28 Jul 2009 15:19:18 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Kevin Winchester <kjwinchester@...il.com>
Cc:	airlied@...hat.com, linux-kernel@...r.kernel.org,
	linux-next@...r.kernel.org
Subject: Re: [PATCH linux-next] agp: correct missing cleanup on error in
 agp_add_bridge

On Sat, 25 Jul 2009 22:30:09 -0300
Kevin Winchester <kjwinchester@...il.com> wrote:

> 
> While investigating a kmemleak detected leak, I encountered the
> agp_add_bridge function.  It appears to be responsible for freeing
> the agp_bridge_data in the case of a failure, but it is only doing
> so for some errors.
> 
> Fix it to always free the bridge data if a failure condition is
> encountered.
> 
> Signed-off-by: Kevin Winchester <kjwinchester@...il.com>
> ---
> 
> Note that this seems like a reasonable change to me, but this patch
> did not get rid of the kmemleak report, so I don't even have that as
> evidence of its correctness.  Please review.
> 
> diff --git a/drivers/char/agp/backend.c b/drivers/char/agp/backend.c
> index cfa5a64..aebd82c 100644
> --- a/drivers/char/agp/backend.c
> +++ b/drivers/char/agp/backend.c
> @@ -263,18 +263,22 @@ int agp_add_bridge(struct agp_bridge_data *bridge)
>  {
>         int error;
>  
> -       if (agp_off)
> -               return -ENODEV;
> +       if (agp_off) {
> +               error = -ENODEV;
> +               goto err_put_bridge;
> +       }
>  
>         if (!bridge->dev) {
>                 printk (KERN_DEBUG PFX "Erk, registering with no pci_dev!\n");
> -               return -EINVAL;
> +               error = -EINVAL;
> +               goto err_put_bridge;
>         }
>  
>         /* Grab reference on the chipset driver. */
>         if (!try_module_get(bridge->driver->owner)) {
>                 dev_info(&bridge->dev->dev, "can't lock chipset driver\n");
> -               return -EINVAL;
> +               error = -EINVAL;
> +               goto err_put_bridge;
>         }
>  
>         error = agp_backend_initialize(bridge);
> @@ -304,6 +308,7 @@ frontend_err:
>         agp_backend_cleanup(bridge);
>  err_out:
>         module_put(bridge->driver->owner);
> +err_put_bridge:
>         agp_put_bridge(bridge);
>         return error;
>  }

Looks right to me.

The code is ill-designed.  The callee (agp_add_bridge) should not be
freeing things which the caller allocated.  Because the callee
shouldn't assume that the caller will no longer use the data and the
callee shouldn't assume that the caller allocated the memory via
kmalloc().  Callers of agp_add_bridge() should be doing the
agp_put_bridge().

Your email client replaced all the tabs with spaces.  I fixed that up.
--
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/