get_fs_type: Validate fs type string argument
PATCH RESEND 1/1 get_fs_type: Validate fs type string argument
On ppc64le, When a string with PAGE_SIZE - 1 (i.e. 64k-1) length is passed as a "filesystem type" argument to the mount(2) syscall, copy_mount_string() ends up allocating 64k (the PAGE_SIZE on ppc64le) worth of space for holding the string in kernel's address space.
Later, in set_precision() (invoked by get_fs_type() -> __request_module() -> vsnprintf()), we end up assigning strlen(fs-type-string) i.e. 65535 as the
value to 'struct printf_spec'->precision member. This field has a width of 16 bits and it is a signed data type. Hence an invalid value ends up getting assigned. This causes the "WARN_ONCE(spec->precision != prec, "precision %d too large", prec)" statement inside set_precision() to be executed.
This commit fixes the bug by validating the length of the "filesystem type" argument passed to get_fs_type() function.
--- fs/filesystems.c | 3 +++ 1 file changed, 3 insertions(+)
fs/filesystems.c +3 -0
@@ -268,6 +268,9 @@ struct file_system_type *get_fs_type(const char *name)const char *dot =strchr (name,'.' );int len = dot ? dot - name :strlen (name); +if (len >=PATH_MAX ) +return NULL ; + fs =__get_fs_type (name, len);if (!fs && (request_module ("fs-%.*s" , len, name) ==0 )) { fs =__get_fs_type (name, len);
Signature · 1 line
2.19.1
Quoted 14 lines · IST Chandan Rajendra
On ppc64le, When a string with
PAGE_SIZE- 1 (i.e. 64k-1) length is passed as a "filesystem type" argument to the mount(2) syscall,copy_mount_string()ends up allocating 64k (thePAGE_SIZEon ppc64le) worth of space for holding the string in kernel's address space.Later, in
set_precision()(invoked byget_fs_type()->__request_module()->vsnprintf()), we end up assigning strlen(fs-type-string) i.e. 65535 as the
value to 'struct printf_spec'->precision member. This field has a width of 16 bits and it is a signed data type. Hence an invalid value ends up getting assigned. This causes the "WARN_ONCE(spec->precision != prec, "precision %d too large", prec)" statement insideset_precision()to be executed.
Quoted 3 lines
This commit fixes the bug by validating the length of the "filesystem type" argument passed to
get_fs_type()function.
The following is a trivial userspace program to recreate the issue,
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mount.h>
#define BUFSIZE 65536
char buf[BUFSIZE];
int main(int argc, char *argv[])
{
int ret;
if (argc != 3) {
fprintf(stderr, "Usage: %s <device> <mount point>.\n", argv[0]);
exit(1);
}
memset(buf, 1, BUFSIZE);
buf[BUFSIZE-1] = '\0';
printf("strlen(buf) = %lu.\n", strlen(buf));
ret = mount(argv[1], argv[2], buf, 0, NULL);
if (ret) {
perror("mount");
exit(0);
}
exit(1);
}Signature · 1 line
chandan
Quoted 15 lines · Chandan Rajendra
On ppc64le, When a string with
PAGE_SIZE- 1 (i.e. 64k-1) length is passed as a "filesystem type" argument to the mount(2) syscall,copy_mount_string()ends up allocating 64k (thePAGE_SIZEon ppc64le) worth of space for holding the string in kernel's address space.Later, in
set_precision()(invoked byget_fs_type()->__request_module()->vsnprintf()), we end up assigning strlen(fs-type-string) i.e. 65535 as the
value to 'struct printf_spec'->precision member. This field has a width of 16 bits and it is a signed data type. Hence an invalid value ends up getting assigned. This causes the "WARN_ONCE(spec->precision != prec, "precision %d too large", prec)" statement insideset_precision()to be executed.
Quoted 6 lines
This commit fixes the bug by validating the length of the "filesystem type" argument passed to
get_fs_type()function.Signed-off-by Chandan Rajendra <[email protected]>Reported-by Abdul Haleem <[email protected]>Suggested-by Joe Perches <[email protected]>
Al, please pick this up, it looks like a good sanity check and lack of it is causing headaches for IBM QA.
Thanks,
Miklos
Quoted 21 lines
--- fs/filesystems.c | 3 +++ 1 file changed, 3 insertions(+)fs/filesystems.c +3
-1@@ -268,6 +268,9 @@ struct file_system_type *get_fs_type(const char *name)const char *dot =strchr (name,'.' );int len = dot ? dot - name :strlen (name); +if (len >=PATH_MAX ) +return NULL ; + fs =__get_fs_type (name, len);if (!fs && (request_module ("fs-%.*s" , len, name) ==0 )) { fs =__get_fs_type (name, len); --2.19.1
Al Viro
maintainerQuoted 20 lines · Miklos Szeredi
Quoted 18 lines · Chandan Rajendra
On ppc64le, When a string with
PAGE_SIZE- 1 (i.e. 64k-1) length is passed as a "filesystem type" argument to the mount(2) syscall,copy_mount_string()ends up allocating 64k (thePAGE_SIZEon ppc64le) worth of space for holding the string in kernel's address space.Later, in
set_precision()(invoked byget_fs_type()->__request_module()->vsnprintf()), we end up assigning strlen(fs-type-string) i.e. 65535 as the
value to 'struct printf_spec'->precision member. This field has a width of 16 bits and it is a signed data type. Hence an invalid value ends up getting assigned. This causes the "WARN_ONCE(spec->precision != prec, "precision %d too large", prec)" statement insideset_precision()to be executed.This commit fixes the bug by validating the length of the "filesystem type" argument passed to
get_fs_type()function.
Quoted 8 lines
Quoted 3 lines
Signed-off-by Chandan Rajendra <[email protected]>Reported-by Abdul Haleem <[email protected]>Suggested-by Joe Perches <[email protected]>Acked-by Miklos Szeredi <[email protected]>Al, please pick this up, it looks like a good sanity check and lack of it is causing headaches for IBM QA.
Umm... I'm not against that patch, but I wonder if that should be caught earlier... If nothing else, the same string is seen by LSM shite before get_fs_type() gets a chance to reject it, and I wouldn't bet a dime on robustness of that code. Wouldn't it be saner to have
char *copy_mount_string(const void __user *data)
{
return data ? strndup_user(data, PAGE_SIZE) : NULL;
}use a more explicit size limit? PATH_MAX, for example?