[2/3] ima: Ensure lock is held when setting iint pointer in inode security blob

Commit Message

Roberto Sassu Oct. 8, 2024, 4:57 p.m. UTC

From: Roberto Sassu <[e-mail]>

IMA stores a pointer of the ima_iint_cache structure, containing integrity
metadata, in the inode security blob. However, check and assignment of this
pointer is not atomic, and it might happen that two tasks both see that the
iint pointer is NULL and try to set it, causing a memory leak.

Ensure that the iint check and assignment is guarded, by adding a lockdep
assertion in ima_inode_get().

Consequently, guard the remaining ima_inode_get() calls, in
ima_post_create_tmpfile() and ima_post_path_mknod(), to avoid the lockdep
warnings.

Signed-off-by: Roberto Sassu <[e-mail]>
---
 security/integrity/ima/ima_iint.c |  5 +++++
 security/integrity/ima/ima_main.c | 14 ++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

Comments

Paul Moore Oct. 9, 2024, 3:41 p.m. UTC | #1

On Tue, Oct 8, 2024 at 12:57 PM Roberto Sassu
<[e-mail]> wrote:
>
> From: Roberto Sassu <[e-mail]>
>
> IMA stores a pointer of the ima_iint_cache structure, containing integrity
> metadata, in the inode security blob. However, check and assignment of this
> pointer is not atomic, and it might happen that two tasks both see that the
> iint pointer is NULL and try to set it, causing a memory leak.
>
> Ensure that the iint check and assignment is guarded, by adding a lockdep
> assertion in ima_inode_get().
>
> Consequently, guard the remaining ima_inode_get() calls, in
> ima_post_create_tmpfile() and ima_post_path_mknod(), to avoid the lockdep
> warnings.
>
> Signed-off-by: Roberto Sassu <[e-mail]>
> ---
>  security/integrity/ima/ima_iint.c |  5 +++++
>  security/integrity/ima/ima_main.c | 14 ++++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
> index c176fd0faae7..fe676ccec32f 100644
> --- a/security/integrity/ima/ima_iint.c
> +++ b/security/integrity/ima/ima_iint.c
> @@ -87,8 +87,13 @@ static void ima_iint_free(struct ima_iint_cache *iint)
>   */
>  struct ima_iint_cache *ima_inode_get(struct inode *inode)
>  {
> +       struct ima_iint_cache_lock *iint_lock;
>         struct ima_iint_cache *iint;
>
> +       iint_lock = ima_inode_security(inode->i_security);
> +       if (iint_lock)
> +               lockdep_assert_held(&iint_lock->mutex);
> +
>         iint = ima_iint_find(inode);
>         if (iint)
>                 return iint;

Can you avoid the ima_iint_find() call here and just do the following?

  /* not sure if you need to check !iint_lock or not? */
  if (!iint_lock)
    return NULL;
  iint = iint_lock->iint;
  if (!iint)
    return NULL;

Roberto Sassu Oct. 9, 2024, 3:43 p.m. UTC | #2

On Wed, 2024-10-09 at 11:41 -0400, Paul Moore wrote:
> On Tue, Oct 8, 2024 at 12:57 PM Roberto Sassu
> <[e-mail]> wrote:
> > 
> > From: Roberto Sassu <[e-mail]>
> > 
> > IMA stores a pointer of the ima_iint_cache structure, containing integrity
> > metadata, in the inode security blob. However, check and assignment of this
> > pointer is not atomic, and it might happen that two tasks both see that the
> > iint pointer is NULL and try to set it, causing a memory leak.
> > 
> > Ensure that the iint check and assignment is guarded, by adding a lockdep
> > assertion in ima_inode_get().
> > 
> > Consequently, guard the remaining ima_inode_get() calls, in
> > ima_post_create_tmpfile() and ima_post_path_mknod(), to avoid the lockdep
> > warnings.
> > 
> > Signed-off-by: Roberto Sassu <[e-mail]>
> > ---
> >  security/integrity/ima/ima_iint.c |  5 +++++
> >  security/integrity/ima/ima_main.c | 14 ++++++++++++--
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
> > index c176fd0faae7..fe676ccec32f 100644
> > --- a/security/integrity/ima/ima_iint.c
> > +++ b/security/integrity/ima/ima_iint.c
> > @@ -87,8 +87,13 @@ static void ima_iint_free(struct ima_iint_cache *iint)
> >   */
> >  struct ima_iint_cache *ima_inode_get(struct inode *inode)
> >  {
> > +       struct ima_iint_cache_lock *iint_lock;
> >         struct ima_iint_cache *iint;
> > 
> > +       iint_lock = ima_inode_security(inode->i_security);
> > +       if (iint_lock)
> > +               lockdep_assert_held(&iint_lock->mutex);
> > +
> >         iint = ima_iint_find(inode);
> >         if (iint)
> >                 return iint;
> 
> Can you avoid the ima_iint_find() call here and just do the following?
> 
>   /* not sure if you need to check !iint_lock or not? */
>   if (!iint_lock)
>     return NULL;
>   iint = iint_lock->iint;
>   if (!iint)
>     return NULL;

Yes, I also like it much more.

Thanks

Roberto

Mimi Zohar Oct. 11, 2024, 7:30 p.m. UTC | #3

On Wed, 2024-10-09 at 17:43 +0200, Roberto Sassu wrote:
> On Wed, 2024-10-09 at 11:41 -0400, Paul Moore wrote:
> > On Tue, Oct 8, 2024 at 12:57 PM Roberto Sassu
> > <[e-mail]> wrote:
> > > 
> > > From: Roberto Sassu <[e-mail]>
> > > 
> > > IMA stores a pointer of the ima_iint_cache structure, containing integrity
> > > metadata, in the inode security blob. However, check and assignment of this
> > > pointer is not atomic, and it might happen that two tasks both see that the
> > > iint pointer is NULL and try to set it, causing a memory leak.
> > > 
> > > Ensure that the iint check and assignment is guarded, by adding a lockdep
> > > assertion in ima_inode_get().
> > > 
> > > Consequently, guard the remaining ima_inode_get() calls, in
> > > ima_post_create_tmpfile() and ima_post_path_mknod(), to avoid the lockdep
> > > warnings.
> > > 
> > > Signed-off-by: Roberto Sassu <[e-mail]>
> > > ---
> > >  security/integrity/ima/ima_iint.c |  5 +++++
> > >  security/integrity/ima/ima_main.c | 14 ++++++++++++--
> > >  2 files changed, 17 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
> > > index c176fd0faae7..fe676ccec32f 100644
> > > --- a/security/integrity/ima/ima_iint.c
> > > +++ b/security/integrity/ima/ima_iint.c
> > > @@ -87,8 +87,13 @@ static void ima_iint_free(struct ima_iint_cache *iint)
> > >   */
> > >  struct ima_iint_cache *ima_inode_get(struct inode *inode)
> > >  {
> > > +       struct ima_iint_cache_lock *iint_lock;
> > >         struct ima_iint_cache *iint;
> > > 
> > > +       iint_lock = ima_inode_security(inode->i_security);
> > > +       if (iint_lock)
> > > +               lockdep_assert_held(&iint_lock->mutex);
> > > +
> > >         iint = ima_iint_find(inode);
> > >         if (iint)
> > >                 return iint;
> > 
> > Can you avoid the ima_iint_find() call here and just do the following?
> > 
> >   /* not sure if you need to check !iint_lock or not? */
> >   if (!iint_lock)
> >     return NULL;
> >   iint = iint_lock->iint;
> >   if (!iint)
> >     return NULL;
> 
> Yes, I also like it much more.

Yes, testing iint_lock and then iint_lock->iint should be fine, but the logic
needs to be inverted.  ima_inode_get() should return the existing iint, if it
exists, or allocate the memory.

Mimi

Roberto Sassu Oct. 14, 2024, 11:45 a.m. UTC | #4

On Fri, 2024-10-11 at 15:30 -0400, Mimi Zohar wrote:
> On Wed, 2024-10-09 at 17:43 +0200, Roberto Sassu wrote:
> > On Wed, 2024-10-09 at 11:41 -0400, Paul Moore wrote:
> > > On Tue, Oct 8, 2024 at 12:57 PM Roberto Sassu
> > > <[e-mail]> wrote:
> > > > 
> > > > From: Roberto Sassu <[e-mail]>
> > > > 
> > > > IMA stores a pointer of the ima_iint_cache structure, containing integrity
> > > > metadata, in the inode security blob. However, check and assignment of this
> > > > pointer is not atomic, and it might happen that two tasks both see that the
> > > > iint pointer is NULL and try to set it, causing a memory leak.
> > > > 
> > > > Ensure that the iint check and assignment is guarded, by adding a lockdep
> > > > assertion in ima_inode_get().
> > > > 
> > > > Consequently, guard the remaining ima_inode_get() calls, in
> > > > ima_post_create_tmpfile() and ima_post_path_mknod(), to avoid the lockdep
> > > > warnings.
> > > > 
> > > > Signed-off-by: Roberto Sassu <[e-mail]>
> > > > ---
> > > >  security/integrity/ima/ima_iint.c |  5 +++++
> > > >  security/integrity/ima/ima_main.c | 14 ++++++++++++--
> > > >  2 files changed, 17 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
> > > > index c176fd0faae7..fe676ccec32f 100644
> > > > --- a/security/integrity/ima/ima_iint.c
> > > > +++ b/security/integrity/ima/ima_iint.c
> > > > @@ -87,8 +87,13 @@ static void ima_iint_free(struct ima_iint_cache *iint)
> > > >   */
> > > >  struct ima_iint_cache *ima_inode_get(struct inode *inode)
> > > >  {
> > > > +       struct ima_iint_cache_lock *iint_lock;
> > > >         struct ima_iint_cache *iint;
> > > > 
> > > > +       iint_lock = ima_inode_security(inode->i_security);
> > > > +       if (iint_lock)
> > > > +               lockdep_assert_held(&iint_lock->mutex);
> > > > +
> > > >         iint = ima_iint_find(inode);
> > > >         if (iint)
> > > >                 return iint;
> > > 
> > > Can you avoid the ima_iint_find() call here and just do the following?
> > > 
> > >   /* not sure if you need to check !iint_lock or not? */
> > >   if (!iint_lock)
> > >     return NULL;
> > >   iint = iint_lock->iint;
> > >   if (!iint)
> > >     return NULL;
> > 
> > Yes, I also like it much more.
> 
> Yes, testing iint_lock and then iint_lock->iint should be fine, but the logic
> needs to be inverted.  ima_inode_get() should return the existing iint, if it
> exists, or allocate the memory.

Right, I checked the patches I'm about to send, they do that.

Thanks

Roberto

Roberto Sassu Oct. 16, 2024, 1:08 p.m. UTC | #5

On Mon, 2024-10-14 at 13:45 +0200, Roberto Sassu wrote:
> On Fri, 2024-10-11 at 15:30 -0400, Mimi Zohar wrote:
> > On Wed, 2024-10-09 at 17:43 +0200, Roberto Sassu wrote:
> > > On Wed, 2024-10-09 at 11:41 -0400, Paul Moore wrote:
> > > > On Tue, Oct 8, 2024 at 12:57 PM Roberto Sassu
> > > > <[e-mail]> wrote:
> > > > > 
> > > > > From: Roberto Sassu <[e-mail]>
> > > > > 
> > > > > IMA stores a pointer of the ima_iint_cache structure, containing integrity
> > > > > metadata, in the inode security blob. However, check and assignment of this
> > > > > pointer is not atomic, and it might happen that two tasks both see that the
> > > > > iint pointer is NULL and try to set it, causing a memory leak.
> > > > > 
> > > > > Ensure that the iint check and assignment is guarded, by adding a lockdep
> > > > > assertion in ima_inode_get().
> > > > > 
> > > > > Consequently, guard the remaining ima_inode_get() calls, in
> > > > > ima_post_create_tmpfile() and ima_post_path_mknod(), to avoid the lockdep
> > > > > warnings.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <[e-mail]>
> > > > > ---
> > > > >  security/integrity/ima/ima_iint.c |  5 +++++
> > > > >  security/integrity/ima/ima_main.c | 14 ++++++++++++--
> > > > >  2 files changed, 17 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
> > > > > index c176fd0faae7..fe676ccec32f 100644
> > > > > --- a/security/integrity/ima/ima_iint.c
> > > > > +++ b/security/integrity/ima/ima_iint.c
> > > > > @@ -87,8 +87,13 @@ static void ima_iint_free(struct ima_iint_cache *iint)
> > > > >   */
> > > > >  struct ima_iint_cache *ima_inode_get(struct inode *inode)
> > > > >  {
> > > > > +       struct ima_iint_cache_lock *iint_lock;
> > > > >         struct ima_iint_cache *iint;
> > > > > 
> > > > > +       iint_lock = ima_inode_security(inode->i_security);
> > > > > +       if (iint_lock)
> > > > > +               lockdep_assert_held(&iint_lock->mutex);
> > > > > +
> > > > >         iint = ima_iint_find(inode);
> > > > >         if (iint)
> > > > >                 return iint;
> > > > 
> > > > Can you avoid the ima_iint_find() call here and just do the following?
> > > > 
> > > >   /* not sure if you need to check !iint_lock or not? */
> > > >   if (!iint_lock)
> > > >     return NULL;
> > > >   iint = iint_lock->iint;
> > > >   if (!iint)
> > > >     return NULL;
> > > 
> > > Yes, I also like it much more.
> > 
> > Yes, testing iint_lock and then iint_lock->iint should be fine, but the logic
> > needs to be inverted.  ima_inode_get() should return the existing iint, if it
> > exists, or allocate the memory.
> 
> Right, I checked the patches I'm about to send, they do that.

I think Paul's point was that we should not create a iint anyway, if
the inode does not have a security blob. That check I think it is fine
to keep.

Roberto

Patch

diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index c176fd0faae7..fe676ccec32f 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -87,8 +87,13 @@  static void ima_iint_free(struct ima_iint_cache *iint)
  */
 struct ima_iint_cache *ima_inode_get(struct inode *inode)
 {
+	struct ima_iint_cache_lock *iint_lock;
 	struct ima_iint_cache *iint;
 
+	iint_lock = ima_inode_security(inode->i_security);
+	if (iint_lock)
+		lockdep_assert_held(&iint_lock->mutex);
+
 	iint = ima_iint_find(inode);
 	if (iint)
 		return iint;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 7852212c43ce..2425067b887d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -705,14 +705,19 @@  static void ima_post_create_tmpfile(struct mnt_idmap *idmap,
 	if (!must_appraise)
 		return;
 
+	ima_iint_lock(inode);
+
 	/* Nothing to do if we can't allocate memory */
 	iint = ima_inode_get(inode);
-	if (!iint)
+	if (!iint) {
+		ima_iint_unlock(inode);
 		return;
+	}
 
 	/* needed for writing the security xattrs */
 	set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
 	iint->ima_file_status = INTEGRITY_PASS;
+	ima_iint_unlock(inode);
 }
 
 /**
@@ -737,13 +742,18 @@  static void ima_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
 	if (!must_appraise)
 		return;
 
+	ima_iint_lock(inode);
+
 	/* Nothing to do if we can't allocate memory */
 	iint = ima_inode_get(inode);
-	if (!iint)
+	if (!iint) {
+		ima_iint_unlock(inode);
 		return;
+	}
 
 	/* needed for re-opening empty files */
 	iint->flags |= IMA_NEW_FILE;
+	ima_iint_unlock(inode);
 }
 
 /**