Description of the issue:
The function 'simple_fill_super()' in the path 'fs/libfs.c' takes the 'magic' argument as int.
In the include file 'include/linux/fs.h' the 's_magic' field of the 'super_block' structure is
declared as unsigned long.
This causes a misbehaviour in the 'Integrity Measurement Architecture' security module,
since the 's_magic' field is used as criteria to determine if the inode must be measured.
This patch applies to the mainline kernel repository.
From a9f6d9bc7b2259ac025977f4b28a8b90784caf62 Mon Sep 17 00:00:00 2001
From: Roberto Sassu <[email protected]>
Date: Tue, 1 Jun 2010 18:28:13 +0200
Subject: [PATCH] BUG: wrong type for magic argument in simple_fill_super(), fs/libfs.c
Signed-off-by: Roberto Sassu <[email protected]>
---
fs/libfs.c | 2 +-
include/linux/fs.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index 09e1016..7d966e8 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -489,7 +489,7 @@ int simple_write_end(struct file *file, struct address_space *mapping,
* unique inode values later for this filesystem, then you must take care
* to pass it an appropriate max_reserved value to avoid collisions.
*/
-int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
+int simple_fill_super(struct super_block *s, unsigned long magic, struct tree_descr *files)
{
struct inode *inode;
struct dentry *root;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3428393..471e1ff 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2388,7 +2388,7 @@ extern const struct file_operations simple_dir_operations;
extern const struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
+extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
--
1.6.6.1
On Tue, 2010-06-01 at 19:05 +0200, Roberto Sassu wrote:
> Description of the issue:
>
> The function 'simple_fill_super()' in the path 'fs/libfs.c' takes the 'magic' argument as int.
> In the include file 'include/linux/fs.h' the 's_magic' field of the 'super_block' structure is
> declared as unsigned long.
> This causes a misbehaviour in the 'Integrity Measurement Architecture' security module,
> since the 's_magic' field is used as criteria to determine if the inode must be measured.
There aren't any magic numbers today greater than 32 bits. Out of
curiosity, which magic number on which platform are you having a
problem?
> This patch applies to the mainline kernel repository.
>
>
> >From a9f6d9bc7b2259ac025977f4b28a8b90784caf62 Mon Sep 17 00:00:00 2001
> From: Roberto Sassu <[email protected]>
> Date: Tue, 1 Jun 2010 18:28:13 +0200
> Subject: [PATCH] BUG: wrong type for magic argument in simple_fill_super(), fs/libfs.c
>
>
> Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
> ---
> fs/libfs.c | 2 +-
> include/linux/fs.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 09e1016..7d966e8 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -489,7 +489,7 @@ int simple_write_end(struct file *file, struct address_space *mapping,
> * unique inode values later for this filesystem, then you must take care
> * to pass it an appropriate max_reserved value to avoid collisions.
> */
> -int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
> +int simple_fill_super(struct super_block *s, unsigned long magic, struct tree_descr *files)
> {
> struct inode *inode;
> struct dentry *root;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3428393..471e1ff 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2388,7 +2388,7 @@ extern const struct file_operations simple_dir_operations;
> extern const struct inode_operations simple_dir_inode_operations;
> struct tree_descr { char *name; const struct file_operations *ops; int mode; };
> struct dentry *d_alloc_name(struct dentry *, const char *);
> -extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
> +extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
> extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
> extern void simple_release_fs(struct vfsmount **mount, int *count);
>
Sorry for resending, the previous was rejected by some mailing list.
On Wednesday 02 June 2010 20:44:25 Mimi Zohar wrote:
> On Tue, 2010-06-01 at 19:05 +0200, Roberto Sassu wrote:
> > Description of the issue:
> >
> > The function 'simple_fill_super()' in the path 'fs/libfs.c' takes the 'magic' argument as int.
> > In the include file 'include/linux/fs.h' the 's_magic' field of the 'super_block' structure is
> > declared as unsigned long.
> > This causes a misbehaviour in the 'Integrity Measurement Architecture' security module,
> > since the 's_magic' field is used as criteria to determine if the inode must be measured.
>
> There aren't any magic numbers today greater than 32 bits. Out of
> curiosity, which magic number on which platform are you having a
> problem?
>
I'm using a Fedora 12 64-bit KVM virtual machine. I do some tests on the 'ima_must_measure()'
function and i noted that the result for inodes with superblock magic SELINUX_MAGIC is to
measure, when the action specified in the default policy is don't measure. So i modified
the code to display the superblock's magic of measured inodes adding this line in the function
'process_measurement()' in 'security/integrity/ima/ima_main.c' after 'ima_must_measure()':
printk("file %s: magic: %lx\n", file->f_dentry->d_name.name, inode->i_sb->s_magic);
I obtained this result:
...
file access: magic 0xfffffffff97cff8c
...
The magic that i'm expecting is 0xf97cff8c. I think this is why the IMA policy is not applied
correctly.
I investigated further the selinux's code to understand how the super_block structure is
instantiated in memory and i found this code in 'security/selinux/selinuxfs.c' , line 1601:
ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files);
In the prototype of the above function the type of the second argument is 'int', when the
's_magic' type of the 'super_block' structure is 'unsigned long'.
In the patch i modified the type of the second argument of the function 'simple_fill_super()'.
This solves my problem but, since this is used by other filesystems, i don't known if this
solution is valid in general.
> > This patch applies to the mainline kernel repository.
> >
> >
> > >From a9f6d9bc7b2259ac025977f4b28a8b90784caf62 Mon Sep 17 00:00:00 2001
> > From: Roberto Sassu <[email protected]>
> > Date: Tue, 1 Jun 2010 18:28:13 +0200
> > Subject: [PATCH] BUG: wrong type for magic argument in simple_fill_super(), fs/libfs.c
> >
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
>
> Reviewed-by: Mimi Zohar <[email protected]>
>
> > ---
> > fs/libfs.c | 2 +-
> > include/linux/fs.h | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index 09e1016..7d966e8 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -489,7 +489,7 @@ int simple_write_end(struct file *file, struct address_space *mapping,
> > * unique inode values later for this filesystem, then you must take care
> > * to pass it an appropriate max_reserved value to avoid collisions.
> > */
> > -int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
> > +int simple_fill_super(struct super_block *s, unsigned long magic, struct tree_descr *files)
> > {
> > struct inode *inode;
> > struct dentry *root;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3428393..471e1ff 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2388,7 +2388,7 @@ extern const struct file_operations simple_dir_operations;
> > extern const struct inode_operations simple_dir_inode_operations;
> > struct tree_descr { char *name; const struct file_operations *ops; int mode; };
> > struct dentry *d_alloc_name(struct dentry *, const char *);
> > -extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
> > +extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
> > extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
> > extern void simple_release_fs(struct vfsmount **mount, int *count);
> >
>
>
>
On Tue, Jun 1, 2010 at 1:05 PM, Roberto Sassu <[email protected]> wrote:
> Description of the issue:
>
> The function 'simple_fill_super()' in the path 'fs/libfs.c' takes the 'magic' argument as int.
> In the include file 'include/linux/fs.h' the 's_magic' field of the 'super_block' structure is
> declared as unsigned long.
> This causes a misbehaviour in the 'Integrity Measurement Architecture' security module,
> since the 's_magic' field is used as criteria to determine if the inode must be measured.
>
> This patch applies to the mainline kernel repository.
>
>
> From a9f6d9bc7b2259ac025977f4b28a8b90784caf62 Mon Sep 17 00:00:00 2001
> From: Roberto Sassu <[email protected]>
> Date: Tue, 1 Jun 2010 18:28:13 +0200
> Subject: [PATCH] BUG: wrong type for magic argument in simple_fill_super(), fs/libfs.c
>
>
> Signed-off-by: Roberto Sassu <[email protected]>
Al,
Can you add the following and push this along to Linus on your next go?
Reviewed-by: Mimi Zohar <[email protected]>
Signed-off-by: Eric Paris <[email protected]>
CC: [email protected]
-Eric
> ---
> ?fs/libfs.c ? ? ? ? | ? ?2 +-
> ?include/linux/fs.h | ? ?2 +-
> ?2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 09e1016..7d966e8 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -489,7 +489,7 @@ int simple_write_end(struct file *file, struct address_space *mapping,
> ?* unique inode values later for this filesystem, then you must take care
> ?* to pass it an appropriate max_reserved value to avoid collisions.
> ?*/
> -int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
> +int simple_fill_super(struct super_block *s, unsigned long magic, struct tree_descr *files)
> ?{
> ? ? ? ?struct inode *inode;
> ? ? ? ?struct dentry *root;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3428393..471e1ff 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2388,7 +2388,7 @@ extern const struct file_operations simple_dir_operations;
> ?extern const struct inode_operations simple_dir_inode_operations;
> ?struct tree_descr { char *name; const struct file_operations *ops; int mode; };
> ?struct dentry *d_alloc_name(struct dentry *, const char *);
> -extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
> +extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
> ?extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
> ?extern void simple_release_fs(struct vfsmount **mount, int *count);
>
> --
> 1.6.6.1
>
On Thu, Jun 03, 2010 at 10:06:27AM -0400, Eric Paris wrote:
> Al,
>
> Can you add the following and push this along to Linus on your next go?
Applied with trivial modification, but... that's not going to make
kABI lovers happy; it's a genuine silent ABI change. Oh, well...