MIME-Version: 1.0
References: <20220106175019.3116389-1-festevam@gmail.com> <Ydc2EHf5f12w4YcW@sirena.org.uk>
In-Reply-To: <Ydc2EHf5f12w4YcW@sirena.org.uk>
From:   Fabio Estevam <festevam@gmail.com>
Date:   Thu, 6 Jan 2022 16:27:32 -0300
Message-ID: <CAOMZO5Czbr=vuvZdqc6+odAQv0M-LJEQVz6uke8OXnoG6wLqwA@mail.gmail.com>
Subject: Re: [PATCH v2] regmap: debugfs: Free debugfs_name buffer after usage
To:     Mark Brown <broonie@kernel.org>
Cc:     Matthias Schiffer <matthias.schiffer@ew.tq-group.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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