Re: [PATCH v2] integrity: Fix possible multiple allocation in integrity_inode_get()

[Tianjia Zhang]

Hi Jarkko,

On 6/9/23 10:24 PM, Jarkko Sakkinen wrote:
> On Thu Jun 1, 2023 at 9:42 AM EEST, Tianjia Zhang wrote:
> > When integrity_inode_get() is querying and inserting the cache, there
> > is a conditional race in the concurrent environment.
> >
> > The race condition is the result of not properly implementing
> > "double-checked locking". In this case, it first checks to see if the
> > iint cache record exists before taking the lock, but doesn't check
> > again after taking the integrity_iint_lock.
> >
> > Fixes: bf2276d10ce5 ("ima: allocating iint improvements")
> > Signed-off-by: Tianjia Zhang <tianjia.zhang@xxxxxxxxxxxxxxxxx>
> > Cc: Dmitry Kasatkin <dmitry.kasatkin@xxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # v3.10+
>
> s/v3.10/v4.14/
>
> I.e. cover only currently maintained longterms, right?

Yes, the race condition was indeed introduced in 3.10, but the fix is
estimated to only cover the LTS version.

> > ---
> >   security/integrity/iint.c | 15 +++++++++------
> >   1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index c73858e8c6d5..a462df827de2 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -43,12 +43,10 @@ static struct integrity_iint_cache *__integrity_iint_find(struct inode *inode)
> >   		else if (inode > iint->inode)
> >   			n = n->rb_right;
> >   		else
> > -			break;
> > +			return iint;
> >   	}
> > -	if (!n)
> > -		return NULL;
> >   
> > -	return iint;
> > +	return NULL;
> >   }
> >   
> >   /*
> > @@ -113,10 +111,15 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
> >   		parent = *p;
> >   		test_iint = rb_entry(parent, struct integrity_iint_cache,
> >   				     rb_node);
> > -		if (inode < test_iint->inode)
> > +		if (inode < test_iint->inode) {
> >   			p = &(*p)->rb_left;
> > -		else
> > +		} else if (inode > test_iint->inode) {
> >   			p = &(*p)->rb_right;
> > +		} else {
> > +			write_unlock(&integrity_iint_lock);
> > +			kmem_cache_free(iint_cache, iint);
> > +			return test_iint;
> > +		}
> >   	}
> >   
> >   	iint->inode = inode;
> > --
> > 2.24.3 (Apple Git-128)
>
> Mimi, are you picking this?

Mimi has picked this patch in next-integrity.

> Off-topic: how do you compile kernel on macOS, you're using VM right?
> I'm just interested because I recently bought Mac mini for both
> compiling and testing arm64. Optimal would be to be able to compile
> the kernel on bare metal and then deploy to a VM...

I am currently only coding and sending the final patch on a Mac.
Compilation and testing are still carried out in the linux environment.
If you have experience in launching a linux VM on macOS, please share it
with me, thanks.

Best regards,
Tianjia