[PATCH] module: ban '.', '..' as module names, ban '/' in module names

[PATCH] module: ban '.', '..' as module names, ban '/' in module names

[Alexey Dobriyan]

As the title says, ban

	.
	..

and any name containing '/' as they show in sysfs as directory names:

	/sys/module/${mod.name}

sysfs tries to mangle the name and make '/' into '!' which kind of work
but not really.

Corrupting simple module to have name '/est' and loading it works:

	# insmod xxx.ko

	$ cat /proc/modules
	/est 12288 0 - Live 0x0000000000000000 (P)

/proc has no problems with it as it ends in data not pathname.

sysfs mangles it to '/sys/module/!test'.

lsmod is confused:

	$ lsmod
	Module                  Size  Used by
	libkmod: ERROR ../libkmod/libkmod-module.c:1998 kmod_module_get_holders: could not open '/sys/module//est/holders': No such file or directory
	/est                      -2  -2

Size and refcount are bogus entirely.

Apparently lsmod doesn't know about sysfs mangling scheme.

Worse, rmmod doesn't work too:

	$ sudo rmmod '/est'
	rmmod: ERROR: Module /est is not currently loaded

I don't even want to know what it is doing.

Practically there is no nice way for the admin to get rid of the module,
so we should just ban such names. Writing small program to just delete
module by name could possibly work maybe.

Any other subsystem should use nice helper function aptly named

	string_is_vfs_ready()

and apply additional restrictions if necessary.

/proc/modules hints that newlines should be banned too,
and \x1f, and whitespace, and similar looking characters 
from different languages and emojis (except 🐧obviously).

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/linux/fs.h   |    8 ++++++++
 kernel/module/main.c |    5 +++++
 2 files changed, 13 insertions(+)

--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3616,4 +3616,12 @@ extern int vfs_fadvise(struct file *file, loff_t offset, loff_t len,
 extern int generic_fadvise(struct file *file, loff_t offset, loff_t len,
 			   int advice);
 
+/*
+ * Use this if data from userspace end up as directory/filename on
+ * some virtual filesystem.
+ */
+static inline bool string_is_vfs_ready(const char *s)
+{
+	return strcmp(s, ".") != 0 && strcmp(s, "..") != 0 && !strchr(s, '/');
+}
 #endif /* _LINUX_FS_H */
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2893,6 +2893,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	audit_log_kern_module(mod->name);
 
+	if (!string_is_vfs_ready(mod->name)) {
+		err = -EINVAL;
+		goto free_module;
+	}
+
 	/* Reserve our place in the list. */
 	err = add_unformed_module(mod);
 	if (err)

Re: [PATCH] module: ban '.', '..' as module names, ban '/' in module names

[Luis Chamberlain]

On Sun, Apr 14, 2024 at 10:05:05PM +0300, Alexey Dobriyan wrote:
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3616,4 +3616,12 @@ extern int vfs_fadvise(struct file *file, loff_t offset, loff_t len,
>  extern int generic_fadvise(struct file *file, loff_t offset, loff_t len,
>  			   int advice);
>  
> +/*
> + * Use this if data from userspace end up as directory/filename on
> + * some virtual filesystem.
> + */
> +static inline bool string_is_vfs_ready(const char *s)
> +{
> +	return strcmp(s, ".") != 0 && strcmp(s, "..") != 0 && !strchr(s, '/');
> +}
>  #endif /* _LINUX_FS_H */
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2893,6 +2893,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  
>  	audit_log_kern_module(mod->name);
>  
> +	if (!string_is_vfs_ready(mod->name)) {
> +		err = -EINVAL;
> +		goto free_module;
> +	}
> +

Sensible change however to put string_is_vfs_ready() in include/linux/fs.h 
is a stretch if there really are no other users.

  Luis

Re: [PATCH] module: ban '.', '..' as module names, ban '/' in module names

[Alexey Dobriyan]

On Sun, Apr 14, 2024 at 01:58:55PM -0700, Luis Chamberlain wrote:
> On Sun, Apr 14, 2024 at 10:05:05PM +0300, Alexey Dobriyan wrote:
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -3616,4 +3616,12 @@ extern int vfs_fadvise(struct file *file, loff_t offset, loff_t len,
> >  extern int generic_fadvise(struct file *file, loff_t offset, loff_t len,
> >  			   int advice);
> >  
> > +/*
> > + * Use this if data from userspace end up as directory/filename on
> > + * some virtual filesystem.
> > + */
> > +static inline bool string_is_vfs_ready(const char *s)
> > +{
> > +	return strcmp(s, ".") != 0 && strcmp(s, "..") != 0 && !strchr(s, '/');
> > +}
> >  #endif /* _LINUX_FS_H */
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -2893,6 +2893,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  
> >  	audit_log_kern_module(mod->name);
> >  
> > +	if (!string_is_vfs_ready(mod->name)) {
> > +		err = -EINVAL;
> > +		goto free_module;
> > +	}
> > +
> 
> Sensible change however to put string_is_vfs_ready() in include/linux/fs.h 
> is a stretch if there really are no other users.

This is forward thinking patch :-)

Other subsystems may create files/directories in proc/sysfs, and should
check for bad names as well:

	/proc/2821/net/dev_snmp6/eth0

This looks exactly like something coming from userspace and making it
into /proc, so the filter function doesn't belong to kernel/module/internal.h

Re: [PATCH] module: ban '.', '..' as module names, ban '/' in module names

[Dr. David Alan Gilbert]

* Alexey Dobriyan (adobriyan@gmail.com) wrote:
> On Sun, Apr 14, 2024 at 01:58:55PM -0700, Luis Chamberlain wrote:
> > On Sun, Apr 14, 2024 at 10:05:05PM +0300, Alexey Dobriyan wrote:
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -3616,4 +3616,12 @@ extern int vfs_fadvise(struct file *file, loff_t offset, loff_t len,
> > >  extern int generic_fadvise(struct file *file, loff_t offset, loff_t len,
> > >  			   int advice);
> > >  
> > > +/*
> > > + * Use this if data from userspace end up as directory/filename on
> > > + * some virtual filesystem.
> > > + */
> > > +static inline bool string_is_vfs_ready(const char *s)
> > > +{
> > > +	return strcmp(s, ".") != 0 && strcmp(s, "..") != 0 && !strchr(s, '/');
> > > +}
> > >  #endif /* _LINUX_FS_H */
> > > --- a/kernel/module/main.c
> > > +++ b/kernel/module/main.c
> > > @@ -2893,6 +2893,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > >  
> > >  	audit_log_kern_module(mod->name);
> > >  
> > > +	if (!string_is_vfs_ready(mod->name)) {
> > > +		err = -EINVAL;
> > > +		goto free_module;
> > > +	}
> > > +
> > 
> > Sensible change however to put string_is_vfs_ready() in include/linux/fs.h 
> > is a stretch if there really are no other users.
> 
> This is forward thinking patch :-)
> 
> Other subsystems may create files/directories in proc/sysfs, and should
> check for bad names as well:
> 
> 	/proc/2821/net/dev_snmp6/eth0
> 
> This looks exactly like something coming from userspace and making it
> into /proc, so the filter function doesn't belong to kernel/module/internal.h

You mean like:

[24180.292204] tuxthe____: renamed from tuxthe🐧
root@dalek:/home/dg# ls /sys/class/net/
enp5s0  lo  tuxthe____  tuxthe🐧  tuxthe🖊  virbr0  virbr1

?

Dave
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH] module: ban '.', '..' as module names, ban '/' in module names

[Matthew Wilcox]

On Sun, Apr 14, 2024 at 10:05:05PM +0300, Alexey Dobriyan wrote:
> Any other subsystem should use nice helper function aptly named
> 
> 	string_is_vfs_ready()
> 
> and apply additional restrictions if necessary.
> 
> /proc/modules hints that newlines should be banned too,
> and \x1f, and whitespace, and similar looking characters 
> from different languages and emojis (except 🐧obviously).

I don't see the purpose of allowing any character in 0x01-0x1f.
How annoying to have BEL in there.  And, really, what's the value in
allowing characters after 0x7e?

Re: [PATCH] module: ban '.', '..' as module names, ban '/' in module names

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] module: ban '.', '..' as module names, ban '/' in module names

[Lucas De Marchi]

On Sun, Apr 14, 2024 at 10:05:05PM +0300, Alexey Dobriyan wrote:
>As the title says, ban
>
>	.
>	..
>
>and any name containing '/' as they show in sysfs as directory names:
>
>	/sys/module/${mod.name}
>
>sysfs tries to mangle the name and make '/' into '!' which kind of work
>but not really.
>
>Corrupting simple module to have name '/est' and loading it works:
>
>	# insmod xxx.ko
>
>	$ cat /proc/modules
>	/est 12288 0 - Live 0x0000000000000000 (P)
>
>/proc has no problems with it as it ends in data not pathname.
>
>sysfs mangles it to '/sys/module/!test'.

did you mean !est?

>
>lsmod is confused:
>
>	$ lsmod
>	Module                  Size  Used by
>	libkmod: ERROR ../libkmod/libkmod-module.c:1998 kmod_module_get_holders: could not open '/sys/module//est/holders': No such file or directory
>	/est                      -2  -2
>
>Size and refcount are bogus entirely.
>
>Apparently lsmod doesn't know about sysfs mangling scheme.

correct

>
>Worse, rmmod doesn't work too:
>
>	$ sudo rmmod '/est'
>	rmmod: ERROR: Module /est is not currently loaded
>
>I don't even want to know what it is doing.

because of the missing sysfs entry above... it first checks if the
module is loaded.

>
>Practically there is no nice way for the admin to get rid of the module,
>so we should just ban such names. Writing small program to just delete
>module by name could possibly work maybe.

--force drops the check for "is it loaded?" and should work

>
>Any other subsystem should use nice helper function aptly named
>
>	string_is_vfs_ready()
>
>and apply additional restrictions if necessary.
>
>/proc/modules hints that newlines should be banned too,
>and \x1f, and whitespace, and similar looking characters
>from different languages and emojis (except 🐧obviously).
>
>Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

But I agree that it would be better to ban these chars from module
names. I don't think we'd ever merge such a module in tree neither.

Lucas De Marchi

>---
>
> include/linux/fs.h   |    8 ++++++++
> kernel/module/main.c |    5 +++++
> 2 files changed, 13 insertions(+)
>
>--- a/include/linux/fs.h
>+++ b/include/linux/fs.h
>@@ -3616,4 +3616,12 @@ extern int vfs_fadvise(struct file *file, loff_t offset, loff_t len,
> extern int generic_fadvise(struct file *file, loff_t offset, loff_t len,
> 			   int advice);
>
>+/*
>+ * Use this if data from userspace end up as directory/filename on
>+ * some virtual filesystem.
>+ */
>+static inline bool string_is_vfs_ready(const char *s)
>+{
>+	return strcmp(s, ".") != 0 && strcmp(s, "..") != 0 && !strchr(s, '/');
>+}
> #endif /* _LINUX_FS_H */
>--- a/kernel/module/main.c
>+++ b/kernel/module/main.c
>@@ -2893,6 +2893,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
>
> 	audit_log_kern_module(mod->name);
>
>+	if (!string_is_vfs_ready(mod->name)) {
>+		err = -EINVAL;
>+		goto free_module;
>+	}
>+
> 	/* Reserve our place in the list. */
> 	err = add_unformed_module(mod);
> 	if (err)