linux-kernel - Re: audit: io_uring openat triggers audit reference count underflow in worker thread

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 7 Oct 2023 07:11:39 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Dan Clash <Dan.Clash@...rosoft.com>,
        "audit@...r.kernel.org" <audit@...r.kernel.org>,
        "io-uring@...r.kernel.org" <io-uring@...r.kernel.org>
Cc:     "paul@...l-moore.com" <paul@...l-moore.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: audit: io_uring openat triggers audit reference count underflow
 in worker thread

On 10/6/23 8:32 PM, Jens Axboe wrote:
> On 10/6/23 2:09 PM, Dan Clash wrote:
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 2a8baa6ce3e8..4f7ac131c9d1 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -187,7 +187,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
>>  		}
>>  	}
>>
>> -	result->refcnt = 1;
>> +	refcount_set(&result->refcnt, 1);
>>  	/* The empty path is special. */
>>  	if (unlikely(!len)) {
>>  		if (empty)
>> @@ -248,7 +248,7 @@ getname_kernel(const char * filename)
>>  	memcpy((char *)result->name, filename, len);
>>  	result->uptr = NULL;
>>  	result->aname = NULL;
>> -	result->refcnt = 1;
>> +	refcount_set(&result->refcnt, 1);
>>  	audit_getname(result);
>>
>>  	return result;
>> @@ -259,9 +259,10 @@ void putname(struct filename *name)
>>  	if (IS_ERR(name))
>>  		return;
>>
>> -	BUG_ON(name->refcnt <= 0);
>> +	BUG_ON(refcount_read(&name->refcnt) == 0);
>> +	BUG_ON(refcount_read(&name->refcnt) == REFCOUNT_SATURATED);
>>
>> -	if (--name->refcnt > 0)
>> +	if (!refcount_dec_and_test(&name->refcnt))
>>  		return;
>>
>>  	if (name->name != name->iname) {
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index d0a54e9aac7a..8217e07726d4 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -2719,7 +2719,7 @@ struct audit_names;
>>  struct filename {
>>  	const char		*name;	/* pointer to actual string */
>>  	const __user char	*uptr;	/* original userland pointer */
>> -	int			refcnt;
>> +	refcount_t		refcnt;
>>  	struct audit_names	*aname;
>>  	const char		iname[];
>>  };
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 37cded22497e..232e0be9f6d9 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -2188,7 +2188,7 @@ __audit_reusename(const __user char *uptr)
>>  		if (!n->name)
>>  			continue;
>>  		if (n->name->uptr == uptr) {
>> -			n->name->refcnt++;
>> +			refcount_inc(&n->name->refcnt);
>>  			return n->name;
>>  		}
>>  	}
>> @@ -2217,7 +2217,7 @@ void __audit_getname(struct filename *name)
>>  	n->name = name;
>>  	n->name_len = AUDIT_NAME_FULL;
>>  	name->aname = n;
>> -	name->refcnt++;
>> +	refcount_inc(&name->refcnt);
>>  }
>>
>>  static inline int audit_copy_fcaps(struct audit_names *name,
>> @@ -2349,7 +2349,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
>>  		return;
>>  	if (name) {
>>  		n->name = name;
>> -		name->refcnt++;
>> +		refcount_inc(&name->refcnt);
>>  	}
>>
>>  out:
>> @@ -2474,7 +2474,7 @@ void __audit_inode_child(struct inode *parent,
>>  		if (found_parent) {
>>  			found_child->name = found_parent->name;
>>  			found_child->name_len = AUDIT_NAME_FULL;
>> -			found_child->name->refcnt++;
>> +			refcount_inc(&found_child->name->refcnt);
>>  		}
>>  	}
> 
> I'm not fully aware of what audit is doing with struct filename outside
> of needing it for the audit log. Rather than impose the atomic
> references for everyone, would it be doable to simply dupe the struct
> instead of grabbing the (non-atomic) reference to the existing one?
> 
> If not, since there's no over/underflow handling right now, it'd
> certainly be cheaper to use an atomic_t here rather than a full
> refcount.

After taking a closer look at this, I think the best course of action
would be to make the struct filename refcnt and atomic_t. With audit in
the picture, it's quite possible to have multiple threads manipulating
the filename refcnt at the same time, which is obviously not currently
safe.

Dan, would you mind sending that as a patch? Include a link to your
original email:

Link: https://lore.kernel.org/lkml/MW2PR2101MB1033FFF044A258F84AEAA584F1C9A@MW2PR2101MB1033.namprd21.prod.outlook.com/

and a Fixes tag as well:

Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")

and CC linux-fsdevel@...r.kernel.org and
Christian Brauner <brauner@...nel.org> as well.

Thanks!

-- 
Jens Axboe