| Message ID | 166365872189.39016.10771273319597352356.stgit@donald.themaw.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | vfs: fix a mount table handling problem | expand |
On Tue, Sep 20, 2022 at 03:26:17PM +0800, Ian Kent wrote: > Whenever a mount has an empty "source" (aka mnt_fsname), the glibc > function getmntent incorrectly parses its input, resulting in reporting > incorrect data to the caller. > > The problem is that the get_mnt_entry() function in glibc's > misc/mntent_r.c assumes that leading whitespace on a line can always > be discarded because it will always be followed by a # for the case > of a comment or a non-whitespace character that's part of the value > of the first field. However, this assumption is violated when the > value of the first field is an empty string. > > This is fixed in the mount API code by simply checking for a pointer > that contains a NULL and treating it as a NULL pointer. Why not simply have the mount API code disallow a zero-length "source" / mnt_fsname? - Ted
On 21/9/22 09:20, Theodore Ts'o wrote: > On Tue, Sep 20, 2022 at 03:26:17PM +0800, Ian Kent wrote: >> Whenever a mount has an empty "source" (aka mnt_fsname), the glibc >> function getmntent incorrectly parses its input, resulting in reporting >> incorrect data to the caller. >> >> The problem is that the get_mnt_entry() function in glibc's >> misc/mntent_r.c assumes that leading whitespace on a line can always >> be discarded because it will always be followed by a # for the case >> of a comment or a non-whitespace character that's part of the value >> of the first field. However, this assumption is violated when the >> value of the first field is an empty string. >> >> This is fixed in the mount API code by simply checking for a pointer >> that contains a NULL and treating it as a NULL pointer. > Why not simply have the mount API code disallow a zero-length "source" > / mnt_fsname? Hi Ted, I suppose but it seems to me that, for certain file systems, mostly pseudo file systems, the source isn't needed and is left out ... so disallowing a zero length source could lead to quite a bit of breakage. Ian
On 21/9/22 12:38, Ian Kent wrote: > > On 21/9/22 09:20, Theodore Ts'o wrote: >> On Tue, Sep 20, 2022 at 03:26:17PM +0800, Ian Kent wrote: >>> Whenever a mount has an empty "source" (aka mnt_fsname), the glibc >>> function getmntent incorrectly parses its input, resulting in reporting >>> incorrect data to the caller. >>> >>> The problem is that the get_mnt_entry() function in glibc's >>> misc/mntent_r.c assumes that leading whitespace on a line can always >>> be discarded because it will always be followed by a # for the case >>> of a comment or a non-whitespace character that's part of the value >>> of the first field. However, this assumption is violated when the >>> value of the first field is an empty string. >>> >>> This is fixed in the mount API code by simply checking for a pointer >>> that contains a NULL and treating it as a NULL pointer. >> Why not simply have the mount API code disallow a zero-length "source" >> / mnt_fsname? > > Hi Ted, > > > I suppose but it seems to me that, for certain file systems, mostly > > pseudo file systems, the source isn't needed and is left out ... so > > disallowing a zero length source could lead to quite a bit of breakage. There's handling consistency too. Ideally any empty string parameter will print "(none)" when listing the proc mount tables. Mostly that happens in the proc code because the field is NULL so if an empty string is specified due to having to provide a positional parameter or for some other reason then handling it by setting the zero length string to NULL in the mount API is conveniently central. We could fix it in the proc code too but then we might see cases get missed over time and we sacrifice an opportunity to improve consistency. To my mind continuing to allow it and dealing with what needs to be done to make that work consistently seemed like the better long term approach. So based on that logic, sticky speaking, the ext patch shouldn't retain the zero length string check but for now ... > > > Ian >
Whenever a mount has an empty "source" (aka mnt_fsname), the glibc function getmntent incorrectly parses its input, resulting in reporting incorrect data to the caller. The problem is that the get_mnt_entry() function in glibc's misc/mntent_r.c assumes that leading whitespace on a line can always be discarded because it will always be followed by a # for the case of a comment or a non-whitespace character that's part of the value of the first field. However, this assumption is violated when the value of the first field is an empty string. This is fixed in the mount API code by simply checking for a pointer that contains a NULL and treating it as a NULL pointer. Changes: v3: added patch to fix zero length string access violation caused after fs parser patch is applied. v2: fix possible oops if conversion functions such as fs_param_is_u32() are called. Signed-off-by: Ian Kent <raven@themaw.net> --- Ian Kent (2): ext4: fix possible null pointer dereference vfs: parse: deal with zero length string value fs/ext4/super.c | 4 ++-- fs/fs_context.c | 17 ++++++++++++----- fs/fs_parser.c | 16 ++++++++++++++++ include/linux/fs_context.h | 3 ++- 4 files changed, 32 insertions(+), 8 deletions(-) -- Ian