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

[Roberto S. <rob...@po...>]

On Wed, 2010-06-02 at 14:44 -0400, 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 <rob...@po...>
> > 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 <rob...@po...>
> 
> Reviewed-by: Mimi Zohar <zo...@us...>
> 
> > ---
> >  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);
> > 
> 
>