linux-kernel - Re: [PATCH v2 000/101] fbdev: Fix memory leak in option parsing

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]

Message-ID: <123794b6-f3e1-7866-f618-4b3534186e80@suse.de>
Date:   Fri, 10 Mar 2023 13:44:30 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     deller@....de, timur@...nel.org, rdunlap@...radead.org,
        paulus@...ba.org, benh@...nel.crashing.org, linux@...linux.org.uk,
        pjones@...hat.com, adaplas@...il.com, s.hauer@...gutronix.de,
        shawnguo@...nel.org, mbroemme@...mpq.org, thomas@...ischhofer.net,
        James.Bottomley@...senpartnership.com, sudipm.mukherjee@...il.com,
        teddy.wang@...iconmotion.com, corbet@....net,
        linux-fbdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 000/101] fbdev: Fix memory leak in option parsing

Hi Geert

Am 10.03.23 um 09:24 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Thu, Mar 9, 2023 at 5:02 PM Thomas Zimmermann <tzimmermann@...e.de> wrote:
>> Introduce struct option_iter and helpers to parse command-line
>> options with comma-separated key-value pairs. Then convert fbdev
>> drivers to the new interface. Fixes a memory leak in the parsing of
>> the video= option.
>>
>> Before commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to
>> caller; clarify ownership"), a call to fb_get_options() either
>> returned an internal string or a duplicated string; hence ownership of
>> the string's memory buffer was not well defined, but depended on how
>> users specified the video= option on the kernel command line. For
>> global settings, the caller owned the returned memory and for per-driver
>> settings, fb_get_options() owned the memory. As calling drivers were
>> unable to detect the case, they had no option but to leak the the memory.
>>
>> Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller;
>> clarify ownership") changed semantics to caller-owned strings. Drivers
>> still leaked the memory, but at least ownership was clear.
> 
> While I can find the actual patch[1], I cannot find this commit?
> Where was it applied?

It's currently in drm-misc-next.

 
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=73ce73c30ba9ae4d90fdfad7ebe9104001d5d851

Best regards
Thomas

> 
> [1] https://lore.kernel.org/all/20230209135509.7786-3-tzimmermann@suse.de
> 
>> This patchset fixes the memory leak and changes string ownership back
>> to fb_get_options(). Patch 1 introduces struct option_iter and a few
>> helpers. The interface takes an option string, such as video=, in the
>> common form value1,key2:value2,value3 etc and returns the individual
>> comma-separated pairs. Various modules use this pattern, so the code
>> is located under lib/.
>>
>> Patches 2 to 100 go through fbdev drivers and convert them to the new
>> interface. This often requires a number of cleanups. A driver would
>> typically refer to the option string's video mode. Such strings are now
>> copied to driver-allocated memory so that drivers don't refer directly
>> to the option string's memory. The option iterator then replaces manual
>> parsing loops based on strsep(","). All driver-allocated memory is
>> released by removing the device or unloading the module.
>>
>> Patch 101 finally changes the ownership of the option string to be
>> internal to fb_get_option(); thereby fixing the memory leak. The option
>> iterator holds its own copy of the string and is not affected by the
>> change.
>>
>> Most fbdev drivers only support to parse option strings if they are
>> built-in. I assume that's because of the original fuzzy semantics of
>> fb_get_options(). A later patchset could change the driver to respect
>> video= settings in any configuration.
>>
>> v2:
>>          * use kstrdup()/kfree() for video strings (Geert, Timur)
>>          * fix iterator docs (Randy)
>>          * update iterator interface
> 
> Thanks for the update, this looks much better!
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)