lkml.org 
[lkml]   [2023]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 3/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()
    On Mon, 13 Nov 2023, Harshit Mogalapalli wrote:

    > 'attr_name_kobj' is allocated using kzalloc, but on all the error paths
    > it is not freed, hence we have a memory leak.
    >
    > Fix the error path before kobject_init_and_add() by adding kfree().
    >
    > kobject_put() must be always called after passing the object to
    > kobject_init_and_add(). Only the error path which is immediately next
    > to kobject_init_and_add() calls kobject_put() and not any other error
    > path after it.
    >
    > Fix the error handling after kobject_init_and_add() by moving the
    > kobject_put() into the goto label err_other_attr_init that is already
    > used by all the error paths after kobject_init_and_add().
    >
    > Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
    > Reported-by: kernel test robot <lkp@intel.com>
    > Reported-by: Dan Carpenter <error27@gmail.com>
    > Closes: https://lore.kernel.org/r/202309201412.on0VXJGo-lkp@intel.com/
    > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
    > ---
    > This is based on static analysis, only compile tested.
    >
    > v3->v4: Add more explicit statement on how we are fixing it, suggested
    > by Ilpo

    Thanks a lot, this looks fine too now.

    Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

    --
    i.


    > ---
    > drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 6 ++++--
    > 1 file changed, 4 insertions(+), 2 deletions(-)
    >
    > diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
    > index a3599498c4e8..6ddca857cc4d 100644
    > --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
    > +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
    > @@ -575,77 +575,79 @@ static void release_attributes_data(void)
    > /**
    > * hp_add_other_attributes() - Initialize HP custom attributes not
    > * reported by BIOS and required to support Secure Platform and Sure
    > * Start.
    > *
    > * @attr_type: Custom HP attribute not reported by BIOS
    > *
    > * Initialize all 2 types of attributes: Platform and Sure Start
    > * object. Populates each attribute types respective properties
    > * under sysfs files.
    > *
    > * Returns zero(0) if successful. Otherwise, a negative value.
    > */
    > static int hp_add_other_attributes(int attr_type)
    > {
    > struct kobject *attr_name_kobj;
    > union acpi_object *obj = NULL;
    > int ret;
    > char *attr_name;
    >
    > attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
    > if (!attr_name_kobj)
    > return -ENOMEM;
    >
    > mutex_lock(&bioscfg_drv.mutex);
    >
    > /* Check if attribute type is supported */
    > switch (attr_type) {
    > case HPWMI_SECURE_PLATFORM_TYPE:
    > attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset;
    > attr_name = SPM_STR;
    > break;
    >
    > case HPWMI_SURE_START_TYPE:
    > attr_name_kobj->kset = bioscfg_drv.main_dir_kset;
    > attr_name = SURE_START_STR;
    > break;
    >
    > default:
    > pr_err("Error: Unknown attr_type: %d\n", attr_type);
    > ret = -EINVAL;
    > - goto err_other_attr_init;
    > + kfree(attr_name_kobj);
    > + goto unlock_drv_mutex;
    > }
    >
    > ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
    > NULL, "%s", attr_name);
    > if (ret) {
    > pr_err("Error encountered [%d]\n", ret);
    > - kobject_put(attr_name_kobj);
    > goto err_other_attr_init;
    > }
    >
    > /* Populate attribute data */
    > switch (attr_type) {
    > case HPWMI_SECURE_PLATFORM_TYPE:
    > ret = hp_populate_secure_platform_data(attr_name_kobj);
    > break;
    >
    > case HPWMI_SURE_START_TYPE:
    > ret = hp_populate_sure_start_data(attr_name_kobj);
    > break;
    >
    > default:
    > ret = -EINVAL;
    > }
    >
    > if (ret)
    > goto err_other_attr_init;
    >
    > mutex_unlock(&bioscfg_drv.mutex);
    > return 0;
    >
    > err_other_attr_init:
    > + kobject_put(attr_name_kobj);
    > +unlock_drv_mutex:
    > mutex_unlock(&bioscfg_drv.mutex);
    > kfree(obj);
    > return ret;
    > }
    >
    \
     
     \ /
      Last update: 2023-11-20 13:58    [W:3.288 / U:0.268 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site