[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

[Mimi Zohar]

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 <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);
> 

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: [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
>

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