linux-kernel - Re: [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse

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:   Fri, 27 Sep 2019 17:10:18 +0200
From:   Cezary Rojewski <cezary.rojewski@...el.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Navid Emamdoost <navid.emamdoost@...il.com>
Cc:     emamd001@....edu, smccaman@....edu, kjlu@....edu,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        Jie Yang <yang.jie@...ux.intel.com>,
        Mark Brown <broonie@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, Enrico Weigelt <info@...ux.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in
 snd_skl_parse_uuids

On 2019-09-27 15:14, Pierre-Louis Bossart wrote:
> On 9/26/19 9:55 PM, Navid Emamdoost wrote:
>> On Wed, Sep 25, 2019 at 12:05:28PM -0500, Pierre-Louis Bossart wrote:
>>> On 9/25/19 11:19 AM, Navid Emamdoost wrote:
>>>> In snd_skl_parse_uuids if allocation for module->instance_id fails, the
>>>> allocated memory for module shoulde be released. I changes the
>>>> allocation for module to use devm_kzalloc to be resource_managed
>>>> allocation and avoid the release in error path.
>>>
>>> if you use devm_, don't you need to fix the error path as well then, 
>>> I see a
>>> kfree(uuid) in skl_freeup_uuid_list().
>>>
>>> I am not very familiar with this code but the error seems to be that the
>>> list_add_tail() is called after the module->instance_id is allocated, so
>>> there is a risk that the module allocated earlier is not freed (since 
>>> it's
>>> not yet added to the list). Freeing the module as done in patch 1 works,
>>> using devm_ without fixing the error path does not seem correct to me.
>>>

Good catch, Pierre.

>> Thanks for the feedback, then it's your call if you can accept patch 1 as
>> fix.
> 
> Cezary, it's really your call.
> 

Actually, not the best person to ask about "objective decisions" here as 
my vision is clouded by changes done internally. This code no longer 
exists in our internal repo. It's better for host to send MODULE_INFO 
request rather than understanding firmware binary structure and parse it 
directly.

I'm fine with solution #1 as I guess asking to wait for refactor is not 
an option. Code deployment is delayed due to range of administrative 
decisions, some of which should be uncovered on alsa-devel soon enough.

Czarek