Hi Stephen, On Fri, 2020-05-08 at 10:14 +1000, Stephen Rothwell wrote: > Hi all, > > On Thu, 30 Apr 2020 21:32:05 +0530 [email protected] wrote: > > > > From: Madhuparna Bhowmik <[email protected]> > > > > This patch fixes the following warning and few other > > instances of traversal of evm_config_xattrnames list: > > > > [ 32.848432] ============================= > > [ 32.848707] WARNING: suspicious RCU usage > > [ 32.848966] 5.7.0-rc1-00006-ga8d5875ce5f0b #1 Not tainted > > [ 32.849308] ----------------------------- > > [ 32.849567] security/integrity/evm/evm_main.c:231 RCU-list traversed in > > non-reader section!! > > > > Since entries are only added to the list and never deleted, > > use list_For_each_entry_lockless() instead of > > list_for_each_entry_rcu() for traversing the list. > > Also, add a relevant comment in evm_secfs.c to indicate this fact. > > > > Reported-by: kernel test robot <[email protected]> > > Suggested-by: Paul E. McKenney <[email protected]> > > Signed-off-by: Madhuparna Bhowmik <[email protected]> > > Acked-by: Paul E. McKenney <[email protected]> > > --- > > security/integrity/evm/evm_crypto.c | 2 +- > > security/integrity/evm/evm_main.c | 4 ++-- > > security/integrity/evm/evm_secfs.c | 9 ++++++++- > > 3 files changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/security/integrity/evm/evm_crypto.c > > b/security/integrity/evm/evm_crypto.c > > index 35682852ddea..b2dc87da5f50 100644 > > --- a/security/integrity/evm/evm_crypto.c > > +++ b/security/integrity/evm/evm_crypto.c > > @@ -207,7 +207,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > > data->hdr.length = crypto_shash_digestsize(desc->tfm); > > > > error = -ENODATA; > > - list_for_each_entry_rcu(xattr, &evm_config_xattrnames, list) { > > + list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) { > > bool is_ima = false; > > > > if (strcmp(xattr->name, XATTR_NAME_IMA) == 0) > > diff --git a/security/integrity/evm/evm_main.c > > b/security/integrity/evm/evm_main.c > > index d361d7fdafc4..0d36259b690d 100644 > > --- a/security/integrity/evm/evm_main.c > > +++ b/security/integrity/evm/evm_main.c > > @@ -97,7 +97,7 @@ static int evm_find_protected_xattrs(struct dentry > > *dentry) > > if (!(inode->i_opflags & IOP_XATTR)) > > return -EOPNOTSUPP; > > > > - list_for_each_entry_rcu(xattr, &evm_config_xattrnames, list) { > > + list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) { > > error = __vfs_getxattr(dentry, inode, xattr->name, NULL, 0); > > if (error < 0) { > > if (error == -ENODATA) > > @@ -228,7 +228,7 @@ static int evm_protected_xattr(const char > > *req_xattr_name) > > struct xattr_list *xattr; > > > > namelen = strlen(req_xattr_name); > > - list_for_each_entry_rcu(xattr, &evm_config_xattrnames, list) { > > + list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) { > > if ((strlen(xattr->name) == namelen) > > && (strncmp(req_xattr_name, xattr->name, namelen) == 0)) { > > found = 1; > > diff --git a/security/integrity/evm/evm_secfs.c > > b/security/integrity/evm/evm_secfs.c > > index 39ad1038d45d..cfc3075769bb 100644 > > --- a/security/integrity/evm/evm_secfs.c > > +++ b/security/integrity/evm/evm_secfs.c > > @@ -232,7 +232,14 @@ static ssize_t evm_write_xattrs(struct file *file, > > const char __user *buf, > > goto out; > > } > > > > - /* Guard against races in evm_read_xattrs */ > > + /* > > + * xattr_list_mutex guards against races in evm_read_xattrs(). > > + * Entries are only added to the evm_config_xattrnames list > > + * and never deleted. Therefore, the list is traversed > > + * using list_for_each_entry_lockless() without holding > > + * the mutex in evm_calc_hmac_or_hash(), evm_find_protected_xattrs() > > + * and evm_protected_xattr(). > > + */ > > mutex_lock(&xattr_list_mutex); > > list_for_each_entry(tmp, &evm_config_xattrnames, list) { > > if (strcmp(xattr->name, tmp->name) == 0) { > > I will apply this to linux-next today. > > Is there something stopping this being applied to a tree? And does it > need to be sent to Linus soon?
Sorry for the delay in pushing this and other fixes to the next- integrity branch. It's in my next-integrity-testing branch. This isn't a bug per-se, just annotating the reason for the lack of locking. thanks, Mimi
