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] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 12 Oct 2023 10:13:39 -0700
From:   "David E. Box" <david.e.box@...ux.intel.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        platform-driver-x86@...r.kernel.org, rajvi.jingar@...ux.intel.com
Subject: Re: [PATCH V3 03/16] platform/x86/intel/vsec: Use cleanup.h

On Thu, 2023-10-12 at 17:46 +0300, Ilpo Järvinen wrote:
> On Wed, 11 Oct 2023, David E. Box wrote:
> 
> > Use cleanup.h helpers to handle cleanup of resources in
> > intel_vsec_add_dev() after failures.
> > 
> > Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> > ---
> > V3 - New patch.
> > 
> >  drivers/platform/x86/intel/vsec.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/vsec.c
> > b/drivers/platform/x86/intel/vsec.c
> > index 15866b7d3117..f03ab89ab7c0 100644
> > --- a/drivers/platform/x86/intel/vsec.c
> > +++ b/drivers/platform/x86/intel/vsec.c
> > @@ -15,6 +15,7 @@
> >  
> >  #include <linux/auxiliary_bus.h>
> >  #include <linux/bits.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/delay.h>
> >  #include <linux/kernel.h>
> >  #include <linux/idr.h>
> > @@ -155,10 +156,10 @@ EXPORT_SYMBOL_NS_GPL(intel_vsec_add_aux, INTEL_VSEC);
> >  static int intel_vsec_add_dev(struct pci_dev *pdev, struct
> > intel_vsec_header *header,
> >                               struct intel_vsec_platform_info *info)
> >  {
> > -       struct intel_vsec_device *intel_vsec_dev;
> > +       struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;
> >         struct resource *res, *tmp;
> >         unsigned long quirks = info->quirks;
> > -       int i;
> > +       int ret, i;
> >  
> >         if (!intel_vsec_supported(header->id, info->caps))
> >                 return -EINVAL;
> > @@ -178,10 +179,8 @@ static int intel_vsec_add_dev(struct pci_dev *pdev,
> > struct intel_vsec_header *he
> >                 return -ENOMEM;
> >  
> >         res = kcalloc(header->num_entries, sizeof(*res), GFP_KERNEL);
> > -       if (!res) {
> > -               kfree(intel_vsec_dev);
> > +       if (!res)
> >                 return -ENOMEM;
> > -       }
> >  
> >         if (quirks & VSEC_QUIRK_TABLE_SHIFT)
> >                 header->offset >>= TABLE_OFFSET_SHIFT;
> > @@ -208,8 +207,12 @@ static int intel_vsec_add_dev(struct pci_dev *pdev,
> > struct intel_vsec_header *he
> >         else
> >                 intel_vsec_dev->ida = &intel_vsec_ida;
> >  
> > -       return intel_vsec_add_aux(pdev, NULL, intel_vsec_dev,
> > -                                 intel_vsec_name(header->id));
> > +       ret = intel_vsec_add_aux(pdev, NULL, intel_vsec_dev,
> > +                                intel_vsec_name(header->id));
> > +
> > +       no_free_ptr(intel_vsec_dev);
> > +
> > +       return ret;
> 
> So if intel_vsec_add_aux() returned an error, intel_vsec_dev won't be 
> freed because of the call call to no_free_ptr() before return. I that's 
> not what you intended?

It will have been freed if intel_vsec_add_aux() fails. It's a little messy. That
function creates the auxdev and assigns the release function which will free
intel_vsec_dev when the device is removed. But there are failure points that
will also invoke the release function. Because of this, for all the failure
points in that function we free intel_vsec_dev so that it's state doesn't need
to be questioned here. This happens explicitly if the failure is before
auxiliary_device_init(), or through the release function invoked by
auxiliary_device_uninit() if after.

David

> 

Powered by blists - more mailing lists