| Message ID | 20191217134748.198011-1-revest@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | integrity: Expose data structures required for include/linux/integrity.h | expand |
On 12/17/2019 5:47 AM, Florent Revest wrote: > From: Florent Revest <revest@google.com> > > include/linux/integrity.h exposes the prototype of integrity_inode_get(). > However, it relies on struct integrity_iint_cache which is currently > defined in an internal header, security/integrity/integrity.h. > > To allow the rest of the kernel to use integrity_inode_get, Why do you want to do this? > this patch > moves the definition of the necessary structures from a private header > to a global kernel header. > > Signed-off-by: Florent Revest <revest@google.com> > --- > include/linux/integrity.h | 37 ++++++++++++++++++++++++++++++++++ > security/integrity/integrity.h | 37 ---------------------------------- > 2 files changed, 37 insertions(+), 37 deletions(-) > > diff --git a/include/linux/integrity.h b/include/linux/integrity.h > index 2271939c5c31..15a0d5e91737 100644 > --- a/include/linux/integrity.h > +++ b/include/linux/integrity.h > @@ -18,6 +18,43 @@ enum integrity_status { > INTEGRITY_UNKNOWN, > }; > > +#define IMA_MAX_DIGEST_SIZE 64 > + > +struct ima_digest_data { > + u8 algo; > + u8 length; > + union { > + struct { > + u8 unused; > + u8 type; > + } sha1; > + struct { > + u8 type; > + u8 algo; > + } ng; > + u8 data[2]; > + } xattr; > + u8 digest[0]; > +} __packed; > + > +/* integrity data associated with an inode */ > +struct integrity_iint_cache { > + struct rb_node rb_node; /* rooted in integrity_iint_tree */ > + struct mutex mutex; /* protects: version, flags, digest */ > + struct inode *inode; /* back pointer to inode in question */ > + u64 version; /* track inode changes */ > + unsigned long flags; > + unsigned long measured_pcrs; > + unsigned long atomic_flags; > + enum integrity_status ima_file_status:4; > + enum integrity_status ima_mmap_status:4; > + enum integrity_status ima_bprm_status:4; > + enum integrity_status ima_read_status:4; > + enum integrity_status ima_creds_status:4; > + enum integrity_status evm_status:4; > + struct ima_digest_data *ima_hash; > +}; > + > /* List of EVM protected security xattrs */ > #ifdef CONFIG_INTEGRITY > extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode); > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index 65377848fbc5..2d5e69ab4646 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -77,25 +77,6 @@ struct evm_ima_xattr_data { > u8 digest[SHA1_DIGEST_SIZE]; > } __packed; > > -#define IMA_MAX_DIGEST_SIZE 64 > - > -struct ima_digest_data { > - u8 algo; > - u8 length; > - union { > - struct { > - u8 unused; > - u8 type; > - } sha1; > - struct { > - u8 type; > - u8 algo; > - } ng; > - u8 data[2]; > - } xattr; > - u8 digest[0]; > -} __packed; > - > /* > * signature format v2 - for using with asymmetric keys > */ > @@ -108,24 +89,6 @@ struct signature_v2_hdr { > uint8_t sig[0]; /* signature payload */ > } __packed; > > -/* integrity data associated with an inode */ > -struct integrity_iint_cache { > - struct rb_node rb_node; /* rooted in integrity_iint_tree */ > - struct mutex mutex; /* protects: version, flags, digest */ > - struct inode *inode; /* back pointer to inode in question */ > - u64 version; /* track inode changes */ > - unsigned long flags; > - unsigned long measured_pcrs; > - unsigned long atomic_flags; > - enum integrity_status ima_file_status:4; > - enum integrity_status ima_mmap_status:4; > - enum integrity_status ima_bprm_status:4; > - enum integrity_status ima_read_status:4; > - enum integrity_status ima_creds_status:4; > - enum integrity_status evm_status:4; > - struct ima_digest_data *ima_hash; > -}; > - > /* rbtree tree calls to lookup, insert, delete > * integrity data associated with an inode. > */
On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote: > On 12/17/2019 5:47 AM, Florent Revest wrote: > > From: Florent Revest <revest@google.com> > > > > include/linux/integrity.h exposes the prototype of integrity_inode_get(). > > However, it relies on struct integrity_iint_cache which is currently > > defined in an internal header, security/integrity/integrity.h. > > > > To allow the rest of the kernel to use integrity_inode_get, > > Why do you want to do this? ditto > > > this patch > > moves the definition of the necessary structures from a private header > > to a global kernel header. > >
On Tue, 2019-12-17 at 18:08 -0500, Mimi Zohar wrote: > On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote: > > On 12/17/2019 5:47 AM, Florent Revest wrote: > > > From: Florent Revest <revest@google.com> > > > > > > include/linux/integrity.h exposes the prototype of > > > integrity_inode_get(). > > > However, it relies on struct integrity_iint_cache which is > > > currently > > > defined in an internal header, security/integrity/integrity.h. > > > > > > To allow the rest of the kernel to use integrity_inode_get, > > > > Why do you want to do this? > > ditto My team works on KRSI (eBPF MAC policies presented at LSS by KP Singh). https://lkml.org/lkml/2019/9/10/393 We identified file hashes gathered from the integrity subsystem as an interesting field that we could potentially someday expose to eBPF programs through helpers. One of the reason behind writing KRSI is to replace a custom kernel auditing module that currently needs to redefine those structures to access them. I imagine other kernel modules could benefit from a file hash API too. This is the least intrusive patch I could come up with that allows us to lookup a hash from an inode. I was surprised to find that integrity_inode_get was exposed but not the structures it returns. If the community is interested in a different file hash API, I'd be happy to iterate on this patch based on your feedback. > > > this patch > > > moves the definition of the necessary structures from a private > > > header > > > to a global kernel header.
On Wed, 2019-12-18 at 12:03 +0100, Florent Revest wrote: > On Tue, 2019-12-17 at 18:08 -0500, Mimi Zohar wrote: > > On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote: > > > On 12/17/2019 5:47 AM, Florent Revest wrote: > > > > From: Florent Revest <revest@google.com> > > > > > > > > include/linux/integrity.h exposes the prototype of > > > > integrity_inode_get(). > > > > However, it relies on struct integrity_iint_cache which is > > > > currently > > > > defined in an internal header, security/integrity/integrity.h. > > > > > > > > To allow the rest of the kernel to use integrity_inode_get, > > > > > > Why do you want to do this? > > > > ditto > > My team works on KRSI (eBPF MAC policies presented at LSS by KP Singh). > https://lkml.org/lkml/2019/9/10/393 We identified file hashes gathered > from the integrity subsystem as an interesting field that we could > potentially someday expose to eBPF programs through helpers. > > One of the reason behind writing KRSI is to replace a custom kernel > auditing module that currently needs to redefine those structures to > access them. I imagine other kernel modules could benefit from a file > hash API too. > > This is the least intrusive patch I could come up with that allows us > to lookup a hash from an inode. I was surprised to find that > integrity_inode_get was exposed but not the structures it returns. > > If the community is interested in a different file hash API, I'd be > happy to iterate on this patch based on your feedback. There's a major difference between returning just the file hash and making the integrity_iint_cache structure public. Peter Moody's original code queried the cache[1]. Why do you need access to the structure itself? FYI, if/when we get to IMA namespacing, the cache structure will change. Mimi [1] ima: add the ability to query ima for the hash of a given file. > > > > > this patch > > > > moves the definition of the necessary structures from a private > > > > header > > > > to a global kernel header. >
[Cc'ing Matthew] On Wed, 2019-12-18 at 08:34 -0500, Mimi Zohar wrote: > On Wed, 2019-12-18 at 12:03 +0100, Florent Revest wrote: > > On Tue, 2019-12-17 at 18:08 -0500, Mimi Zohar wrote: > > > On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote: > > > > On 12/17/2019 5:47 AM, Florent Revest wrote: > > > > > From: Florent Revest <revest@google.com> > > > > > > > > > > include/linux/integrity.h exposes the prototype of > > > > > integrity_inode_get(). > > > > > However, it relies on struct integrity_iint_cache which is > > > > > currently > > > > > defined in an internal header, security/integrity/integrity.h. > > > > > > > > > > To allow the rest of the kernel to use integrity_inode_get, > > > > > > > > Why do you want to do this? > > > > > > ditto > > > > My team works on KRSI (eBPF MAC policies presented at LSS by KP Singh). > > https://lkml.org/lkml/2019/9/10/393 We identified file hashes gathered > > from the integrity subsystem as an interesting field that we could > > potentially someday expose to eBPF programs through helpers. > > > > One of the reason behind writing KRSI is to replace a custom kernel > > auditing module that currently needs to redefine those structures to > > access them. I imagine other kernel modules could benefit from a file > > hash API too. > > > > This is the least intrusive patch I could come up with that allows us > > to lookup a hash from an inode. I was surprised to find that > > integrity_inode_get was exposed but not the structures it returns. > > > > If the community is interested in a different file hash API, I'd be > > happy to iterate on this patch based on your feedback. > > There's a major difference between returning just the file hash and > making the integrity_iint_cache structure public. Peter Moody's > original code queried the cache[1]. Why do you need access to the > structure itself? > > FYI, if/when we get to IMA namespacing, the cache structure will > change. > > [1] ima: add the ability to query ima for the hash of a given file. If you're using Peter's patch, or something similar, I'd appreciate your taking the time to upstream it. Mimi > > > > > > > > this patch > > > > > moves the definition of the necessary structures from a private > > > > > header > > > > > to a global kernel header. > > >
On Wed, 2019-12-18 at 09:28 -0500, Mimi Zohar wrote: > [Cc'ing Matthew] > > On Wed, 2019-12-18 at 08:34 -0500, Mimi Zohar wrote: > > On Wed, 2019-12-18 at 12:03 +0100, Florent Revest wrote: > > > On Tue, 2019-12-17 at 18:08 -0500, Mimi Zohar wrote: > > > > On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote: > > > > > On 12/17/2019 5:47 AM, Florent Revest wrote: > > > > > > From: Florent Revest <revest@google.com> > > > > > > > > > > > > include/linux/integrity.h exposes the prototype of > > > > > > integrity_inode_get(). > > > > > > However, it relies on struct integrity_iint_cache which is > > > > > > currently > > > > > > defined in an internal header, > > > > > > security/integrity/integrity.h. > > > > > > > > > > > > To allow the rest of the kernel to use integrity_inode_get, > > > > > > > > > > Why do you want to do this? > > > > > > > > ditto > > > > > > My team works on KRSI (eBPF MAC policies presented at LSS by KP > > > Singh). > > > https://lkml.org/lkml/2019/9/10/393 We identified file hashes > > > gathered > > > from the integrity subsystem as an interesting field that we > > > could > > > potentially someday expose to eBPF programs through helpers. > > > > > > One of the reason behind writing KRSI is to replace a custom > > > kernel > > > auditing module that currently needs to redefine those structures > > > to > > > access them. I imagine other kernel modules could benefit from a > > > file > > > hash API too. > > > > > > This is the least intrusive patch I could come up with that > > > allows us > > > to lookup a hash from an inode. I was surprised to find that > > > integrity_inode_get was exposed but not the structures it > > > returns. > > > > > > If the community is interested in a different file hash API, I'd > > > be > > > happy to iterate on this patch based on your feedback. > > > > There's a major difference between returning just the file hash and > > making the integrity_iint_cache structure public. Certainly! I am new to this subsystem so I just wanted to get the discussion started. I am happy to make a more specific function. > > Peter Moody's original code queried the cache[1]. Why do you need > > access to the structure itself? > > FYI, if/when we get to IMA namespacing, the cache structure will > > change. > > > > [1] ima: add the ability to query ima for the hash of a given file. > > If you're using Peter's patch, or something similar, I'd appreciate > your taking the time to upstream it. Thank you for pointing me to Peter's patch! No one in my team was aware of his work on this. Ugh! It appears that Peter left the company while trying to upstream his patch and the situation just got stuck there for 4+ years now. If you are still positive about the idea of a ima_file_hash function, I will take his v6 patch (this is the latest I could find on the sourceforce archives of linux-ima-devel), rebase it, take your comments into account and send a new version by the end of the week. > Mimi > > > > > > > this patch > > > > > > moves the definition of the necessary structures from a > > > > > > private > > > > > > header > > > > > > to a global kernel header.
On Wed, 2019-12-18 at 17:56 +0100, Florent Revest wrote: > On Wed, 2019-12-18 at 09:28 -0500, Mimi Zohar wrote: > > [Cc'ing Matthew] > > > > > There's a major difference between returning just the file hash and > > > making the integrity_iint_cache structure public. > > Certainly! > I am new to this subsystem so I just wanted to get the discussion > started. I am happy to make a more specific function. > > > > Peter Moody's original code queried the cache[1]. Why do you need > > > access to the structure itself? > > > FYI, if/when we get to IMA namespacing, the cache structure will > > > change. > > > > > > [1] ima: add the ability to query ima for the hash of a given file. > > > > If you're using Peter's patch, or something similar, I'd appreciate > > your taking the time to upstream it. > > Thank you for pointing me to Peter's patch! No one in my team was aware > of his work on this. Ugh! > It appears that Peter left the company while trying to upstream his > patch and the situation just got stuck there for 4+ years now. > > If you are still positive about the idea of a ima_file_hash function, I > will take his v6 patch (this is the latest I could find on the > sourceforce archives of linux-ima-devel), rebase it, take your comments > into account and send a new version by the end of the week. Matthew also wasn't aware of Peter's patch, until I sent it to him. I assume they're using it or something similar. Please coordinate with him, before refreshing and posting the patch. thanks, Mimi
diff --git a/include/linux/integrity.h b/include/linux/integrity.h index 2271939c5c31..15a0d5e91737 100644 --- a/include/linux/integrity.h +++ b/include/linux/integrity.h @@ -18,6 +18,43 @@ enum integrity_status { INTEGRITY_UNKNOWN, }; +#define IMA_MAX_DIGEST_SIZE 64 + +struct ima_digest_data { + u8 algo; + u8 length; + union { + struct { + u8 unused; + u8 type; + } sha1; + struct { + u8 type; + u8 algo; + } ng; + u8 data[2]; + } xattr; + u8 digest[0]; +} __packed; + +/* integrity data associated with an inode */ +struct integrity_iint_cache { + struct rb_node rb_node; /* rooted in integrity_iint_tree */ + struct mutex mutex; /* protects: version, flags, digest */ + struct inode *inode; /* back pointer to inode in question */ + u64 version; /* track inode changes */ + unsigned long flags; + unsigned long measured_pcrs; + unsigned long atomic_flags; + enum integrity_status ima_file_status:4; + enum integrity_status ima_mmap_status:4; + enum integrity_status ima_bprm_status:4; + enum integrity_status ima_read_status:4; + enum integrity_status ima_creds_status:4; + enum integrity_status evm_status:4; + struct ima_digest_data *ima_hash; +}; + /* List of EVM protected security xattrs */ #ifdef CONFIG_INTEGRITY extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode); diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 65377848fbc5..2d5e69ab4646 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -77,25 +77,6 @@ struct evm_ima_xattr_data { u8 digest[SHA1_DIGEST_SIZE]; } __packed; -#define IMA_MAX_DIGEST_SIZE 64 - -struct ima_digest_data { - u8 algo; - u8 length; - union { - struct { - u8 unused; - u8 type; - } sha1; - struct { - u8 type; - u8 algo; - } ng; - u8 data[2]; - } xattr; - u8 digest[0]; -} __packed; - /* * signature format v2 - for using with asymmetric keys */ @@ -108,24 +89,6 @@ struct signature_v2_hdr { uint8_t sig[0]; /* signature payload */ } __packed; -/* integrity data associated with an inode */ -struct integrity_iint_cache { - struct rb_node rb_node; /* rooted in integrity_iint_tree */ - struct mutex mutex; /* protects: version, flags, digest */ - struct inode *inode; /* back pointer to inode in question */ - u64 version; /* track inode changes */ - unsigned long flags; - unsigned long measured_pcrs; - unsigned long atomic_flags; - enum integrity_status ima_file_status:4; - enum integrity_status ima_mmap_status:4; - enum integrity_status ima_bprm_status:4; - enum integrity_status ima_read_status:4; - enum integrity_status ima_creds_status:4; - enum integrity_status evm_status:4; - struct ima_digest_data *ima_hash; -}; - /* rbtree tree calls to lookup, insert, delete * integrity data associated with an inode. */