[PATCH][1/1] fs: wrong type for 'magic' argument in 'simple_fill_super()', fs/libfs.c

[PATCH][1/1] fs: wrong type for 'magic' argument in 'simple_fill_super()', fs/libfs.c

[Roberto Sassu]

[-- Attachment #1: Type: Text/Plain, Size: 2282 bytes --]

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 <roberto.sassu@polito.it>
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 <roberto.sassu@polito.it>
---
 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

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2153 bytes --]

Re: [Linux-ima-user] [PATCH][1/1] fs: wrong type for 'magic' argument in 'simple_fill_super()', fs/libfs.c

[Roberto Sassu]

[-- Attachment #1: Type: Text/Plain, Size: 4256 bytes --]

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 <roberto.sassu@polito.it>
> > 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 <roberto.sassu@polito.it>
> 
> Reviewed-by: Mimi Zohar <zohar@us.ibm.com>
> 
> > ---
> >  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);
> > 
> 
> 
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2153 bytes --]

Re: [PATCH][1/1] fs: wrong type for 'magic' argument in 'simple_fill_super()', fs/libfs.c

[Eric Paris]

On Tue, Jun 1, 2010 at 1:05 PM, Roberto Sassu <roberto.sassu@polito.it> 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 <roberto.sassu@polito.it>
> 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 <roberto.sassu@polito.it>

Al,

Can you add the following and push this along to Linus on your next go?

Reviewed-by: Mimi Zohar <zohar@us.ibm.com>
Signed-off-by: Eric Paris <eparis@redhat.com>
CC: stable@kernel.org

-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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][1/1] fs: wrong type for 'magic' argument in 'simple_fill_super()', fs/libfs.c

[Al Viro]

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...