lkml.org 
[lkml]   [2022]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2] regmap: debugfs: Free debugfs_name buffer after usage
Hi Mark,

On Thu, Jan 6, 2022 at 3:33 PM Mark Brown <broonie@kernel.org> wrote:

> OK, but what's the logic here? The name is getting thrown away here but

I did more debugging and this is what I found:
The 'debugfs: Directory 'dummy-iomuxc-gpr@20e0000' with parent
'regmap' already present!' message
happens since commit cffa4b2122f5 ("regmap: debugfs: Fix a memory leak
when calling regmap_attach_dev").

Prior to this commit map->debugfs_name would always be allocated via
kasprintf().

After this commit, the allocation only happens when !map->debugfs_name.

This matches with my observations:

- The first allocation for dummy-iomuxc-gpr@20e0000 works as
map->debugfs_name is NULL.
- The second time map->debugfs_name is not NULL, so there is no allocation
via kasprintf(), and the map->debugfs_name uses the 'old' entry from
the previous buffer.

This causes the directory name to be duplicated and fails to be created.

That's why clearing map->debugfs_name causes a new kasprintf()
allocation and restores the correct behavior.

Prior to cffa4b2122f5 ("regmap: debugfs: Fix a memory leak when
calling regmap_attach_dev"):

# mount -t debugfs none /sys/kernel/debug/
# cat /sys/kernel/debug/regmap/dummy-iomuxc-gpr@20e0000/registers
00: 00000000
04: 48611005
08: 0000000c
0c: 01e00000
10: f00000cf
14: 0000000e
18: 007f007f
1c: 007f007f
20: fffd4000
24: 00000000
28: 00003800
2c: 00000000
30: 0f004490
34: 0593e4a4
# cat /sys/kernel/debug/regmap/20e0000.pinctrl-gpr/registers
00: 00000000
04: 48611005
08: 0000000c
0c: 01e00000
10: f00000cf
14: 00000007
18: 007f007f
1c: 007f007f
20: fffd4000
24: 00000000
28: 00003800
2c: 00000000
30: 0f004490
34: 0593e4a4

After commit cffa4b2122f5 ("regmap: debugfs: Fix a memory leak when
calling regmap_attach_dev):

The 'debugfs: Directory 'dummy-iomuxc-gpr@20e0000' with parent
'regmap' already present!' message is seen.

# cat /sys/kernel/debug/regmap/20e0000.pinctrl-gpr/registers
cat: can't open
'/sys/kernel/debug/regmap/20e0000.pinctrl-gpr/registers': No such file
or directory

# cat /sys/kernel/debug/regmap/dummy-iomuxc-gpr@20e0000/registers
00: 00000000
04: 48611005
08: 0000000c
0c: 01e00000
10: f00000cf
14: 00000009
18: 007f007f
1c: 007f007f
20: fffd4000
24: 00000000
28: 00003800
2c: 00000000
30: 0f004490
34: 0593e4a4

> clearly there is a file still so I'm not seeing how anything is going to
> clean that file up. That means that if the device gets freed we'll end
> up with the old debugfs file hanging around pointing at nothing. Like I
> said (originally in response to Matthias' patch but pasted in this
> thread as well):
>
> | (we should probably clean up the one with no device but that's not what
> | your commit does). I think what you need to look at here is that we
>
> The use after free extends beyond just the filename, we're also loosing
> track of the already created file, which does seem to be an existing
> bug. To be more explicit this means we need a call to regmap_debugfs_exit()
> which will clean up all the existing debugfs stuff before we loose
> references to it.

As shown above, I don't see the '
/sys/kernel/debug/regmap/20e0000.pinctrl-gpr' directory being created,
so there is nothing to clean.

Where exactly would you like me to call regmap_debugfs_exit()?

Thanks

\
 
 \ /
  Last update: 2022-01-06 20:28    [W:0.330 / U:0.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site