linux-kernel - Re: [Linux-ima-user] [PATCH][1/1] fs: wrong type for 'magic' argument in 'simple_fill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201006031158.33629.roberto.sassu@polito.it>
Date:	Thu, 3 Jun 2010 11:58:28 +0200
From:	Roberto Sassu <roberto.sassu@...ito.it>
To:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:	linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org, James Morris <jmorris@...ei.org>,
	linux-ima-user@...ts.sourceforge.net, linux-fsdevel@...r.kernel.org
Subject: Re: [Linux-ima-user] [PATCH][1/1] fs: wrong type for 'magic' 
 argument	in 'simple_fill_super()', fs/libfs.c

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@...ito.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@...ito.it>
> 
> Reviewed-by: Mimi Zohar <zohar@...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);
> > 
> 
> 
> 

Download attachment "smime.p7s" of type "application/pkcs7-signature" (2153 bytes)