[PATCH] xfs: include the XFS magic number in magic.h

[PATCH] xfs: include the XFS magic number in magic.h

[Mimi Zohar]

Move the XFS_SB_MAGIC definition to magic.h.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 fs/xfs/libxfs/xfs_format.h | 3 ++-
 include/uapi/linux/magic.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 1acb584fc5f7..05f7d83e5f88 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -18,6 +18,8 @@
 #ifndef __XFS_FORMAT_H__
 #define __XFS_FORMAT_H__
 
+#include <linux/magic.h>
+
 /*
  * XFS On Disk Format Definitions
  *
@@ -38,7 +40,6 @@ struct xfs_ifork;
  * Fits into a sector-sized buffer at address 0 of each allocation group.
  * Only the first of these is ever updated except during growfs.
  */
-#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
 #define	XFS_SB_VERSION_1	1		/* 5.3, 6.0.1, 6.1 */
 #define	XFS_SB_VERSION_2	2		/* 6.2 - attributes */
 #define	XFS_SB_VERSION_3	3		/* 6.2 - new inode version */
diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
index 1a6fee974116..f6a641826e79 100644
--- a/include/uapi/linux/magic.h
+++ b/include/uapi/linux/magic.h
@@ -67,6 +67,7 @@
 #define TRACEFS_MAGIC          0x74726163
 
 #define V9FS_MAGIC		0x01021997
+#define XFS_SB_MAGIC		0x58465342
 
 #define BDEVFS_MAGIC            0x62646576
 #define DAXFS_MAGIC             0x64646178
-- 
2.7.4

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Christoph Hellwig]

On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> Move the XFS_SB_MAGIC definition to magic.h.

NACK.  We want to keep the XFS code self-contained and no other part
of the kernel has any business knowing it anyway.

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Mimi Zohar]

On Tue, 2017-12-12 at 06:26 -0800, Christoph Hellwig wrote:
> On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> > Move the XFS_SB_MAGIC definition to magic.h.
> 
> NACK.  We want to keep the XFS code self-contained and no other part
> of the kernel has any business knowing it anyway.

IMA policy rules can be defined in terms of magic numbers, but they
need to be defined in magic.h.  Please reconsider...

Mimi

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Christoph Hellwig]

On Tue, Dec 12, 2017 at 09:34:56AM -0500, Mimi Zohar wrote:
> On Tue, 2017-12-12 at 06:26 -0800, Christoph Hellwig wrote:
> > On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> > > Move the XFS_SB_MAGIC definition to magic.h.
> > 
> > NACK.  We want to keep the XFS code self-contained and no other part
> > of the kernel has any business knowing it anyway.
> 
> IMA policy rules can be defined in terms of magic numbers, but they
> need to be defined in magic.h.  Please reconsider...

That is completely bogus, and it should not be supported in any way.
File systems magic numbers are internal implementation details.

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Mimi Zohar]

On Tue, 2017-12-12 at 06:36 -0800, Christoph Hellwig wrote:
> On Tue, Dec 12, 2017 at 09:34:56AM -0500, Mimi Zohar wrote:
> > On Tue, 2017-12-12 at 06:26 -0800, Christoph Hellwig wrote:
> > > On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> > > > Move the XFS_SB_MAGIC definition to magic.h.
> > > 
> > > NACK.  We want to keep the XFS code self-contained and no other part
> > > of the kernel has any business knowing it anyway.
> > 
> > IMA policy rules can be defined in terms of magic numbers, but they
> > need to be defined in magic.h.  Please reconsider...
> 
> That is completely bogus, and it should not be supported in any way.
> File systems magic numbers are internal implementation details.

Perhaps policies in general shouldn't differentiate between file
systems, but it definitely simplifies testing.

For example, currently IMA-appraisal only supports storing file
signatures as xattrs, but support for appended signatures is being
added.  Per file system rules could require different types of file
signatures.

Mimi

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Dave Chinner]

On Tue, Dec 12, 2017 at 10:04:56AM -0500, Mimi Zohar wrote:
> On Tue, 2017-12-12 at 06:36 -0800, Christoph Hellwig wrote:
> > On Tue, Dec 12, 2017 at 09:34:56AM -0500, Mimi Zohar wrote:
> > > On Tue, 2017-12-12 at 06:26 -0800, Christoph Hellwig wrote:
> > > > On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> > > > > Move the XFS_SB_MAGIC definition to magic.h.
> > > > 
> > > > NACK.  We want to keep the XFS code self-contained and no other part
> > > > of the kernel has any business knowing it anyway.
> > > 
> > > IMA policy rules can be defined in terms of magic numbers, but they
> > > need to be defined in magic.h.  Please reconsider...
> > 
> > That is completely bogus, and it should not be supported in any way.
> > File systems magic numbers are internal implementation details.
> 
> Perhaps policies in general shouldn't differentiate between file
> systems, but it definitely simplifies testing.

How does specifying a filesystem type in a generic layer's policy
implementation help testing?

> For example, currently IMA-appraisal only supports storing file
> signatures as xattrs, but support for appended signatures is being
> added.  Per file system rules could require different types of file
> signatures.

What's an "appended signature", and why is the filesystem
independent xattr interface insufficient for storing this
information?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Mimi Zohar]

On Wed, 2017-12-13 at 10:30 +1100, Dave Chinner wrote:
> On Tue, Dec 12, 2017 at 10:04:56AM -0500, Mimi Zohar wrote:
> > On Tue, 2017-12-12 at 06:36 -0800, Christoph Hellwig wrote:
> > > On Tue, Dec 12, 2017 at 09:34:56AM -0500, Mimi Zohar wrote:
> > > > On Tue, 2017-12-12 at 06:26 -0800, Christoph Hellwig wrote:
> > > > > On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> > > > > > Move the XFS_SB_MAGIC definition to magic.h.
> > > > > 
> > > > > NACK.  We want to keep the XFS code self-contained and no other part
> > > > > of the kernel has any business knowing it anyway.
> > > > 
> > > > IMA policy rules can be defined in terms of magic numbers, but they
> > > > need to be defined in magic.h.  Please reconsider...
> > > 
> > > That is completely bogus, and it should not be supported in any way.
> > > File systems magic numbers are internal implementation details.
> > 
> > Perhaps policies in general shouldn't differentiate between file
> > systems, but it definitely simplifies testing.
> 
> How does specifying a filesystem type in a generic layer's policy
> implementation help testing?

Based on file system, I could differentiate which files need to be
signed.  For example, the root file system might require files
signatures only on executables, while for other file systems all files
could require signatures.

> > For example, currently IMA-appraisal only supports storing file
> > signatures as xattrs, but support for appended signatures is being
> > added.  Per file system rules could require different types of file
> > signatures.
> 
> What's an "appended signature", and why is the filesystem
> independent xattr interface insufficient for storing this
> information?

Unfortunately, not all filesystems support xattrs (eg.
cpio/initramfs).  In some cases, like kexec kernel image and
initramfs, having an attached signatures simplifies network boot.
  
scripts/sign-file is used to append a file signature to kernel
modules.  This same script could be used for signing other files, like
the kexec kernel image and initramfs.

Mimi

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Dave Chinner]

On Tue, Dec 12, 2017 at 06:55:31PM -0500, Mimi Zohar wrote:
> On Wed, 2017-12-13 at 10:30 +1100, Dave Chinner wrote:
> > On Tue, Dec 12, 2017 at 10:04:56AM -0500, Mimi Zohar wrote:
> > > On Tue, 2017-12-12 at 06:36 -0800, Christoph Hellwig wrote:
> > > > On Tue, Dec 12, 2017 at 09:34:56AM -0500, Mimi Zohar wrote:
> > > > > On Tue, 2017-12-12 at 06:26 -0800, Christoph Hellwig wrote:
> > > > > > On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> > > > > > > Move the XFS_SB_MAGIC definition to magic.h.
> > > > > > 
> > > > > > NACK.  We want to keep the XFS code self-contained and no other part
> > > > > > of the kernel has any business knowing it anyway.
> > > > > 
> > > > > IMA policy rules can be defined in terms of magic numbers, but they
> > > > > need to be defined in magic.h.  Please reconsider...
> > > > 
> > > > That is completely bogus, and it should not be supported in any way.
> > > > File systems magic numbers are internal implementation details.
> > > 
> > > Perhaps policies in general shouldn't differentiate between file
> > > systems, but it definitely simplifies testing.
> > 
> > How does specifying a filesystem type in a generic layer's policy
> > implementation help testing?
> 
> Based on file system, I could differentiate which files need to be
> signed.  For example, the root file system might require files
> signatures only on executables, while for other file systems all files
> could require signatures.

What's the filesystem magic number got to do with where the
filesystem is mounted or what it contains?

> > > For example, currently IMA-appraisal only supports storing file
> > > signatures as xattrs, but support for appended signatures is being
> > > added.  Per file system rules could require different types of file
> > > signatures.
> > 
> > What's an "appended signature", and why is the filesystem
> > independent xattr interface insufficient for storing this
> > information?
> 
> Unfortunately, not all filesystems support xattrs (eg.
> cpio/initramfs).

So add support to them....

>  In some cases, like kexec kernel image and
> initramfs, having an attached signatures simplifies network boot.

... or support fetching detached signatures for these very specific
use cases.

> scripts/sign-file is used to append a file signature to kernel
> modules.  This same script could be used for signing other files, like
> the kexec kernel image and initramfs.

This doesn't require knowing about what type of filesystem the file
is read from. It's just data appended to the file, and you can
already read filesystem without knowing what the underlying
filesystem implementation is....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Mimi Zohar]

On Wed, 2017-12-13 at 11:13 +1100, Dave Chinner wrote:
> On Tue, Dec 12, 2017 at 06:55:31PM -0500, Mimi Zohar wrote:
> > On Wed, 2017-12-13 at 10:30 +1100, Dave Chinner wrote:
> > > On Tue, Dec 12, 2017 at 10:04:56AM -0500, Mimi Zohar wrote:
> > > > On Tue, 2017-12-12 at 06:36 -0800, Christoph Hellwig wrote:
> > > > > On Tue, Dec 12, 2017 at 09:34:56AM -0500, Mimi Zohar wrote:
> > > > > > On Tue, 2017-12-12 at 06:26 -0800, Christoph Hellwig wrote:
> > > > > > > On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> > > > > > > > Move the XFS_SB_MAGIC definition to magic.h.
> > > > > > > 
> > > > > > > NACK.  We want to keep the XFS code self-contained and no other part
> > > > > > > of the kernel has any business knowing it anyway.
> > > > > > 
> > > > > > IMA policy rules can be defined in terms of magic numbers, but they
> > > > > > need to be defined in magic.h.  Please reconsider...
> > > > > 
> > > > > That is completely bogus, and it should not be supported in any way.
> > > > > File systems magic numbers are internal implementation details.
> > > > 
> > > > Perhaps policies in general shouldn't differentiate between file
> > > > systems, but it definitely simplifies testing.
> > > 
> > > How does specifying a filesystem type in a generic layer's policy
> > > implementation help testing?
> > 
> > Based on file system, I could differentiate which files need to be
> > signed.  For example, the root file system might require files
> > signatures only on executables, while for other file systems all files
> > could require signatures.
> 
> What's the filesystem magic number got to do with where the
> filesystem is mounted or what it contains?
> 
> > > > For example, currently IMA-appraisal only supports storing file
> > > > signatures as xattrs, but support for appended signatures is being
> > > > added.  Per file system rules could require different types of file
> > > > signatures.
> > > 
> > > What's an "appended signature", and why is the filesystem
> > > independent xattr interface insufficient for storing this
> > > information?
> > 
> > Unfortunately, not all filesystems support xattrs (eg.
> > cpio/initramfs).
> 
> So add support to them....

Definitely, that would be the best solution!  Anyone have time and is
interested in adding xattr support to CPIO?

> >  In some cases, like kexec kernel image and
> > initramfs, having an attached signatures simplifies network boot.
> 
> ... or support fetching detached signatures for these very specific
> use cases.

That would require changes to the protocol (eg. PXE).

> > scripts/sign-file is used to append a file signature to kernel
> > modules.  This same script could be used for signing other files, like
> > the kexec kernel image and initramfs.
> 
> This doesn't require knowing about what type of filesystem the file
> is read from. It's just data appended to the file, and you can
> already read filesystem without knowing what the underlying
> filesystem implementation is....

Agreed, this is in response to your question "What's an appended
signature".  The context was defining policy rules for testing, which
either require different types of files signatures or require
signatures for different file types, on a per file system basis.

Sigh, this patch was not suppose to be controversial.  Most, if not
all, of the other file systems are included in magic.h.

Mimi

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Dave Chinner]

On Tue, Dec 12, 2017 at 08:21:12PM -0500, Mimi Zohar wrote:
> On Wed, 2017-12-13 at 11:13 +1100, Dave Chinner wrote:
> > On Tue, Dec 12, 2017 at 06:55:31PM -0500, Mimi Zohar wrote:
> > > Based on file system, I could differentiate which files need to be
> > > signed.  For example, the root file system might require files
> > > signatures only on executables, while for other file systems all files
> > > could require signatures.
> > 
> > What's the filesystem magic number got to do with where the
> > filesystem is mounted or what it contains?

<silence>

[....]

> > > Unfortunately, not all filesystems support xattrs (eg.
> > > cpio/initramfs).
> > 
> > So add support to them....
> 
> Definitely, that would be the best solution!  Anyone have time and is
> interested in adding xattr support to CPIO?

One who needs the functionality finds the resources....

> > > scripts/sign-file is used to append a file signature to kernel
> > > modules.  This same script could be used for signing other files, like
> > > the kexec kernel image and initramfs.
> > 
> > This doesn't require knowing about what type of filesystem the file
> > is read from. It's just data appended to the file, and you can
> > already read filesystem without knowing what the underlying
> > filesystem implementation is....
> 
> Agreed, this is in response to your question "What's an appended
> signature".

Sure, but ...

>  The context was defining policy rules for testing, which
> either require different types of files signatures or require
> signatures for different file types, on a per file system basis.

... you still haven't explained why you need the filesystem magic
number for this or even why you need to have different
methods/policies for different filesystem types.

> Sigh, this patch was not suppose to be controversial.

It's not controversial, it's just wrong. :)

There are around 40 defined magic numbers in the XFS on-disk format.
They all get defined in the XFS on-disk format headers and should
not be used outside the XFS code.

>  Most, if not
> all, of the other file systems are included in magic.h.

And your point is?  Seriously, using filesystem magic numbers in the
kernel for detecting filesystems is misguided because:

	a) they don't uniquely identify a filesystem, and

	b) superblocks already have a struct file_system_type
	(sb->s_type) attached to them that is used explicity for
	identifying the filesystem type *by name*.

IOWs, if you need to identify a filesystem *type*, using internal
magic numbers is not the way to do it. selinux, for example, checks
sb->s_type->name quite a lot....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Christoph Hellwig]

On Wed, Dec 13, 2017 at 01:59:28PM +1100, Dave Chinner wrote:
> IOWs, if you need to identify a filesystem *type*, using internal
> magic numbers is not the way to do it. selinux, for example, checks
> sb->s_type->name quite a lot....

And that is bad enough to start with.  It should check for specific
features, and not the file system driver name.

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Mimi Zohar]

On Wed, 2017-12-13 at 13:59 +1100, Dave Chinner wrote:

> IOWs, if you need to identify a filesystem *type*, using internal
> magic numbers is not the way to do it. selinux, for example, checks
> sb->s_type->name quite a lot....

Thanks, this works fine.  Is there a defined max name length?

Mimi

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Dave Chinner]

On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> Move the XFS_SB_MAGIC definition to magic.h.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  fs/xfs/libxfs/xfs_format.h | 3 ++-
>  include/uapi/linux/magic.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 1acb584fc5f7..05f7d83e5f88 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -18,6 +18,8 @@
>  #ifndef __XFS_FORMAT_H__
>  #define __XFS_FORMAT_H__
>  
> +#include <linux/magic.h>
> +
>  /*
>   * XFS On Disk Format Definitions
>   *
> @@ -38,7 +40,6 @@ struct xfs_ifork;
>   * Fits into a sector-sized buffer at address 0 of each allocation group.
>   * Only the first of these is ever updated except during growfs.
>   */
> -#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
>  #define	XFS_SB_VERSION_1	1		/* 5.3, 6.0.1, 6.1 */
>  #define	XFS_SB_VERSION_2	2		/* 6.2 - attributes */
>  #define	XFS_SB_VERSION_3	3		/* 6.2 - new inode version */

NAK.

The XFS superblock magic number is part of the on-disk format
definition of XFS. It belongs with all the other on-disk format
definitions in this file.

Besides, there is almost never a good reason to be doing stuff based
on filesystem magic numbers. What, exactly, do you need to know
filesystem magic numbers for in the kernel?

Cheers,

Dave.


-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: include the XFS magic number in magic.h

[Mimi Zohar]

Hi Dave,

On Wed, 2017-12-13 at 08:13 +1100, Dave Chinner wrote:
> On Tue, Dec 12, 2017 at 09:21:09AM -0500, Mimi Zohar wrote:
> > Move the XFS_SB_MAGIC definition to magic.h.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > ---
> >  fs/xfs/libxfs/xfs_format.h | 3 ++-
> >  include/uapi/linux/magic.h | 1 +
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index 1acb584fc5f7..05f7d83e5f88 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -18,6 +18,8 @@
> >  #ifndef __XFS_FORMAT_H__
> >  #define __XFS_FORMAT_H__
> >  
> > +#include <linux/magic.h>
> > +
> >  /*
> >   * XFS On Disk Format Definitions
> >   *
> > @@ -38,7 +40,6 @@ struct xfs_ifork;
> >   * Fits into a sector-sized buffer at address 0 of each allocation group.
> >   * Only the first of these is ever updated except during growfs.
> >   */
> > -#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
> >  #define	XFS_SB_VERSION_1	1		/* 5.3, 6.0.1, 6.1 */
> >  #define	XFS_SB_VERSION_2	2		/* 6.2 - attributes */
> >  #define	XFS_SB_VERSION_3	3		/* 6.2 - new inode version */
> 
> NAK.
> 
> The XFS superblock magic number is part of the on-disk format
> definition of XFS. It belongs with all the other on-disk format
> definitions in this file.
> 
> Besides, there is almost never a good reason to be doing stuff based
> on filesystem magic numbers. What, exactly, do you need to know
> filesystem magic numbers for in the kernel?

IMA measures, appraises, and audits files based on policy.  The
builtin in measurement and appraisal policies can be defined on the
boot command line (eg. ima_policy=tcb|appraise_tcb|secure_boot).
 These initial builtin policies are course grain. 

After the LSMs are enabled, the IMA policy can be defined in terms of
the LSM labels and other file metadata (eg. UUID, uid, euid, fsmagic,
...).  Normally the builtin policies are subsequently replaced with a
custom policy in the initramfs.

Mimi