| 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
| ||
|
Message-ID: <6cc3d747-94ed-d494-1cda-fd054747c62b@gmx.de>
Date: Mon, 20 Mar 2023 20:25:39 +0100
From: Helge Deller <deller@....de>
To: Thomas Zimmermann <tzimmermann@...e.de>, geert+renesas@...der.be,
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
Cc: 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 Thomas,
On 3/20/23 11:07, Thomas Zimmermann wrote:
> Geert, Helge? Do you have further comments? There's not really much for a v3 yet.
I understand the motivation and I see you invested a lot of work on it,
which is really appreciated.
But I have mixed feelings about that patch itself.
Nevertheless, it mixes multiple things.
Regarding the possible memory leak (for the parameter) you added in various
places a kfree(), but this doesn't make it easier for the driver author to know
when to free and when not.
I wonder if it would be possible to store the kstrdup() value in the
"struct module" (or somewhere module related) instead add kfree it at a central place when the
module is unloaded (instead inside the driver itself). That way no driver needs
to be touched and there are less changes necessary.
And it would change it back to globally-owned strings.
With such a change I think the new command parsing functions wouldn't be needed
either, and ideally, where possible, a conversion to module_param() in various
places would be better.
I'm sure I haven't overlooked everything yet, but I think if we can reduce
the necessary changes by fixing things globally it makes more sense.
Helge
>
> Best regards
> Thomas
>
> Am 09.03.23 um 17:00 schrieb Thomas Zimmermann:
>> 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.
>>
>> 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
>>
>> Thomas Zimmermann (101):
>> lib: Add option iterator
>> fbdev/68328fb: Remove trailing whitespaces
>> fbdev/68328fb: Remove unused option string
>> fbdev/acornfb: Only init fb_info once
>> fbdev/acornfb: Parse option string with struct option_iter
>> fbdev/amifb: Duplicate video-mode option string
>> fbdev/amifb: Parse option string with struct option_iter
>> fbdev/arkfb: Duplicate video-mode option string
>> fbdev/atafb: Duplicate video-mode option string
>> fbdev/atafb: Parse option string with struct option_iter
>> fbdev/aty: Duplicate video-mode option string
>> fbdev/aty: Parse option string with struct option_iter
>> fbdev/au1100fb: Parse option string with struct option_iter
>> fbdev/au1200fb: Parse option string with struct option_iter
>> fbdev/cirrusfb: Duplicate video-mode option string
>> fbdev/cirrusfb: Parse option string with struct option_iter
>> fbdev/controlfb: Remove trailing whitespaces
>> fbdev/controlfb: Parse option string with struct option_iter
>> fbdev/cyber2000fb: Parse option string with struct option_iter
>> fbdev/efifb: Parse option string with struct option_iter
>> fbdev/fm2fb: Parse option string with struct option_iter
>> fbdev/fsl-diu-fb: Duplicate video-mode option string
>> fbdev/fsl-diu-fb: Parse option string with struct option_iter
>> fbdev/gbefb: Duplicate video-mode option string
>> fbdev/gbefb: Parse option string with struct option_iter
>> fbdev/geode: Duplicate video-mode option string
>> fbdev/geode: Parse option string with struct option_iter
>> fbdev/grvga: Duplicate video-mode option string
>> fbdev/grvga: Parse option string with struct option_iter
>> fbdev/gxt4500: Duplicate video-mode option string
>> fbdev/hyperv_fb: Duplicate video-mode option string
>> fbdev/i740fb: Duplicate video-mode option string
>> fbdev/i740fb: Parse option string with struct option_iter
>> fbdev/i810: Duplicate video-mode option string
>> fbdev/i810: Parse option string with struct option_iter
>> fbdev/imsttfb: Parse option string with struct option_iter
>> fbdev/intelfb: Duplicate video-mode option string
>> fbdev/intelfb: Parse option string with struct option_iter
>> fbdev/imxfb: Duplicate video-mode option string
>> fbdev/imxfb: Parse option string with struct option_iter
>> fbdev/kyrofb: Duplicate video-mode option string
>> fbdev/kyrofb: Parse option string with struct option_iter
>> fbdev/macfb: Remove trailing whitespaces
>> fbdev/macfb: Parse option string with struct option_iter
>> fbdev/matroxfb: Parse option string with struct option_iter
>> fbdev/mx3fb: Duplicate video-mode option string
>> fbdev/mx3fb: Parse option string with struct option_iter
>> fbdev/neofb: Duplicate video-mode option string
>> fbdev/neofb: Parse option string with struct option_iter
>> fbdev/nvidiafb: Duplicate video-mode option string
>> fbdev/nvidiafb: Parse option string with struct option_iter
>> fbdev/ocfb: Duplicate video-mode option string
>> fbdev/ocfb: Parse option string with struct option_iter
>> fbdev/omapfb: Parse option string with struct option_iter
>> fbdev/platinumfb: Remove trailing whitespaces
>> fbdev/platinumfb: Parse option string with struct option_iter
>> fbdev/pm2fb: Duplicate video-mode option string
>> fbdev/pm2fb: Parse option string with struct option_iter
>> fbdev/pm3fb: Duplicate video-mode option string
>> fbdev/pm3fb: Parse option string with struct option_iter
>> fbdev/ps3fb: Duplicate video-mode option string
>> fbdev/ps3fb: Parse option string with struct option_iter
>> fbdev/pvr2fb: Duplicate video-mode option string
>> fbdev/pvr2fb: Parse option string with struct option_iter
>> fbdev/pxafb: Parse option string with struct option_iter
>> fbdev/rivafb: Duplicate video-mode option string
>> fbdev/rivafb: Parse option string with struct option_iter
>> fbdev/s3fb: Duplicate video-mode option string
>> fbdev/s3fb: Parse option string with struct option_iter
>> fbdev/savagefb: Duplicate video-mode option string
>> fbdev/savagefb: Parse option string with struct option_iter
>> fbdev/sisfb: Constify mode string
>> fbdev/sisfb: Parse option string with struct option_iter
>> fbdev/skeletonfb: Parse option string with struct option_iter
>> fbdev/sm712fb: Duplicate video-mode option string
>> fbdev/sstfb: Duplicate video-mode option string
>> fbdev/sstfb: Parse option string with struct option_iter
>> fbdev/stifb: Remove trailing whitespaces
>> fbdev/stifb: Constify option string
>> fbdev/tdfxfb: Duplicate video-mode option string
>> fbdev/tdfxfb: Parse option string with struct option_iter
>> fbdev/tgafb: Duplicate video-mode option string
>> fbdev/tgafb: Parse option string with struct option_iter
>> fbdev/tmiofb: Remove unused option string
>> fbdev/tridentfb: Duplicate video-mode option string
>> fbdev/tridentfb: Parse option string with struct option_iter
>> fbdev/uvesafb: Duplicate video-mode option string
>> fbdev/uvesafb: Parse option string with struct option_iter
>> fbdev/valkyriefb: Remove trailing whitespaces
>> fbdev/valkyriefb: Parse option string with struct option_iter
>> fbdev/vermilion: Remove unused option string
>> fbdev/vesafb: Parse option string with struct option_iter
>> fbdev/vfb: Remove trailing whitespaces
>> fbdev/vfb: Duplicate video-mode option string
>> fbdev/vfb: Parse option string with struct option_iter
>> fbdev/viafb: Parse option string with struct option_iter
>> fbdev/vt8623fb: Duplicate video-mode option string
>> staging/sm750fb: Release g_settings in module-exit function
>> staging/sm750fb: Duplicate video-mode option string
>> staging/sm750fb: Parse option string with struct option_iter
>> fbdev: Constify option strings
>>
>> Documentation/core-api/kernel-api.rst | 9 ++
>> drivers/staging/sm750fb/sm750.c | 63 ++++----
>> drivers/video/fbdev/68328fb.c | 24 +--
>> drivers/video/fbdev/acornfb.c | 23 ++-
>> drivers/video/fbdev/amifb.c | 23 +--
>> drivers/video/fbdev/arkfb.c | 10 +-
>> drivers/video/fbdev/atafb.c | 21 +--
>> drivers/video/fbdev/aty/aty128fb.c | 22 ++-
>> drivers/video/fbdev/aty/atyfb_base.c | 23 ++-
>> drivers/video/fbdev/aty/radeon_base.c | 26 +--
>> drivers/video/fbdev/au1100fb.c | 13 +-
>> drivers/video/fbdev/au1200fb.c | 15 +-
>> drivers/video/fbdev/cirrusfb.c | 30 ++--
>> drivers/video/fbdev/controlfb.c | 47 +++---
>> drivers/video/fbdev/core/fb_cmdline.c | 13 +-
>> drivers/video/fbdev/core/modedb.c | 8 +-
>> drivers/video/fbdev/cyber2000fb.c | 17 +-
>> drivers/video/fbdev/efifb.c | 44 ++---
>> drivers/video/fbdev/ep93xx-fb.c | 2 +-
>> drivers/video/fbdev/fm2fb.c | 14 +-
>> drivers/video/fbdev/fsl-diu-fb.c | 24 +--
>> drivers/video/fbdev/gbefb.c | 23 +--
>> drivers/video/fbdev/geode/gx1fb_core.c | 16 +-
>> drivers/video/fbdev/geode/gxfb_core.c | 23 +--
>> drivers/video/fbdev/geode/lxfb_core.c | 25 +--
>> drivers/video/fbdev/grvga.c | 18 ++-
>> drivers/video/fbdev/gxt4500.c | 13 +-
>> drivers/video/fbdev/hyperv_fb.c | 18 ++-
>> drivers/video/fbdev/i740fb.c | 26 +--
>> drivers/video/fbdev/i810/i810_main.c | 26 ++-
>> drivers/video/fbdev/imsttfb.c | 16 +-
>> drivers/video/fbdev/imxfb.c | 21 +--
>> drivers/video/fbdev/intelfb/intelfbdrv.c | 23 ++-
>> drivers/video/fbdev/kyro/fbdev.c | 21 ++-
>> drivers/video/fbdev/macfb.c | 26 +--
>> drivers/video/fbdev/matrox/matroxfb_base.c | 19 +--
>> drivers/video/fbdev/mx3fb.c | 23 ++-
>> drivers/video/fbdev/neofb.c | 26 +--
>> drivers/video/fbdev/nvidia/nvidia.c | 26 ++-
>> drivers/video/fbdev/ocfb.c | 21 ++-
>> drivers/video/fbdev/omap/omapfb_main.c | 15 +-
>> drivers/video/fbdev/platinumfb.c | 44 ++---
>> drivers/video/fbdev/pm2fb.c | 25 +--
>> drivers/video/fbdev/pm3fb.c | 27 ++--
>> drivers/video/fbdev/ps3fb.c | 28 ++--
>> drivers/video/fbdev/pvr2fb.c | 32 ++--
>> drivers/video/fbdev/pxafb.c | 18 ++-
>> drivers/video/fbdev/riva/fbdev.c | 26 ++-
>> drivers/video/fbdev/s3fb.c | 27 ++--
>> drivers/video/fbdev/savage/savagefb_driver.c | 20 ++-
>> drivers/video/fbdev/sis/sis_main.c | 24 +--
>> drivers/video/fbdev/skeletonfb.c | 17 +-
>> drivers/video/fbdev/sm712fb.c | 12 +-
>> drivers/video/fbdev/sstfb.c | 25 +--
>> drivers/video/fbdev/stifb.c | 162 +++++++++----------
>> drivers/video/fbdev/tdfxfb.c | 21 ++-
>> drivers/video/fbdev/tgafb.c | 30 ++--
>> drivers/video/fbdev/tmiofb.c | 24 +--
>> drivers/video/fbdev/tridentfb.c | 27 ++--
>> drivers/video/fbdev/uvesafb.c | 21 ++-
>> drivers/video/fbdev/valkyriefb.c | 30 ++--
>> drivers/video/fbdev/vermilion/vermilion.c | 7 +-
>> drivers/video/fbdev/vesafb.c | 16 +-
>> drivers/video/fbdev/vfb.c | 35 ++--
>> drivers/video/fbdev/via/viafbdev.c | 15 +-
>> drivers/video/fbdev/vt8623fb.c | 11 +-
>> include/linux/cmdline.h | 36 +++++
>> include/linux/fb.h | 2 +-
>> lib/Makefile | 2 +-
>> lib/cmdline_iter.c | 109 +++++++++++++
>> 70 files changed, 1087 insertions(+), 682 deletions(-)
>> create mode 100644 include/linux/cmdline.h
>> create mode 100644 lib/cmdline_iter.c
>>
>
Powered by blists - more mailing lists