[5/9] LSM: Introduce inode_post_setattr hook
Commit Message
IMA and EVM need to hook after setattr finishes. Introduce this hook and
move IMA and EVM's open-coded stacking to use it.
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jonathan McDowell <noodles@fb.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/attr.c | 3 +--
include/linux/evm.h | 6 ------
include/linux/ima.h | 9 ---------
include/linux/lsm_hook_defs.h | 3 +++
security/integrity/evm/evm_main.c | 10 +++++++++-
security/integrity/ima/ima.h | 2 ++
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_main.c | 1 +
security/security.c | 8 ++++++++
9 files changed, 25 insertions(+), 19 deletions(-)
Comments
Hi Kees,
I love your patch! Yet something to improve:
[auto build test ERROR on kees/for-next/pstore]
[also build test ERROR on pcmoore-audit/next pcmoore-selinux/next linus/master v6.1-rc1 next-20221017]
[cannot apply to zohar-integrity/next-integrity]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/integrity-Move-hooks-into-LSM/20221014-063953
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
patch link: https://lore.kernel.org/r/20221013223654.659758-5-keescook%40chromium.org
patch subject: [PATCH 5/9] LSM: Introduce inode_post_setattr hook
config: m68k-randconfig-r002-20221012
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4a05d49d9e6e09539a85db353b335221f1181f38
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kees-Cook/integrity-Move-hooks-into-LSM/20221014-063953
git checkout 4a05d49d9e6e09539a85db353b335221f1181f38
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
fs/attr.c: In function 'notify_change':
>> fs/attr.c:426:17: error: implicit declaration of function 'security_inode_post_setattr'; did you mean 'security_inode_post_setxattr'? [-Werror=implicit-function-declaration]
426 | security_inode_post_setattr(mnt_userns, dentry, ia_valid);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
| security_inode_post_setxattr
cc1: some warnings being treated as errors
vim +426 fs/attr.c
290
291 /**
292 * notify_change - modify attributes of a filesytem object
293 * @mnt_userns: user namespace of the mount the inode was found from
294 * @dentry: object affected
295 * @attr: new attributes
296 * @delegated_inode: returns inode, if the inode is delegated
297 *
298 * The caller must hold the i_mutex on the affected object.
299 *
300 * If notify_change discovers a delegation in need of breaking,
301 * it will return -EWOULDBLOCK and return a reference to the inode in
302 * delegated_inode. The caller should then break the delegation and
303 * retry. Because breaking a delegation may take a long time, the
304 * caller should drop the i_mutex before doing so.
305 *
306 * Alternatively, a caller may pass NULL for delegated_inode. This may
307 * be appropriate for callers that expect the underlying filesystem not
308 * to be NFS exported. Also, passing NULL is fine for callers holding
309 * the file open for write, as there can be no conflicting delegation in
310 * that case.
311 *
312 * If the inode has been found through an idmapped mount the user namespace of
313 * the vfsmount must be passed through @mnt_userns. This function will then
314 * take care to map the inode according to @mnt_userns before checking
315 * permissions. On non-idmapped mounts or if permission checking is to be
316 * performed on the raw inode simply passs init_user_ns.
317 */
318 int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
319 struct iattr *attr, struct inode **delegated_inode)
320 {
321 struct inode *inode = dentry->d_inode;
322 umode_t mode = inode->i_mode;
323 int error;
324 struct timespec64 now;
325 unsigned int ia_valid = attr->ia_valid;
326
327 WARN_ON_ONCE(!inode_is_locked(inode));
328
329 error = may_setattr(mnt_userns, inode, ia_valid);
330 if (error)
331 return error;
332
333 if ((ia_valid & ATTR_MODE)) {
334 umode_t amode = attr->ia_mode;
335 /* Flag setting protected by i_mutex */
336 if (is_sxid(amode))
337 inode->i_flags &= ~S_NOSEC;
338 }
339
340 now = current_time(inode);
341
342 attr->ia_ctime = now;
343 if (!(ia_valid & ATTR_ATIME_SET))
344 attr->ia_atime = now;
345 else
346 attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
347 if (!(ia_valid & ATTR_MTIME_SET))
348 attr->ia_mtime = now;
349 else
350 attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
351
352 if (ia_valid & ATTR_KILL_PRIV) {
353 error = security_inode_need_killpriv(dentry);
354 if (error < 0)
355 return error;
356 if (error == 0)
357 ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV;
358 }
359
360 /*
361 * We now pass ATTR_KILL_S*ID to the lower level setattr function so
362 * that the function has the ability to reinterpret a mode change
363 * that's due to these bits. This adds an implicit restriction that
364 * no function will ever call notify_change with both ATTR_MODE and
365 * ATTR_KILL_S*ID set.
366 */
367 if ((ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID)) &&
368 (ia_valid & ATTR_MODE))
369 BUG();
370
371 if (ia_valid & ATTR_KILL_SUID) {
372 if (mode & S_ISUID) {
373 ia_valid = attr->ia_valid |= ATTR_MODE;
374 attr->ia_mode = (inode->i_mode & ~S_ISUID);
375 }
376 }
377 if (ia_valid & ATTR_KILL_SGID) {
378 if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
379 if (!(ia_valid & ATTR_MODE)) {
380 ia_valid = attr->ia_valid |= ATTR_MODE;
381 attr->ia_mode = inode->i_mode;
382 }
383 attr->ia_mode &= ~S_ISGID;
384 }
385 }
386 if (!(attr->ia_valid & ~(ATTR_KILL_SUID | ATTR_KILL_SGID)))
387 return 0;
388
389 /*
390 * Verify that uid/gid changes are valid in the target
391 * namespace of the superblock.
392 */
393 if (ia_valid & ATTR_UID &&
394 !vfsuid_has_fsmapping(mnt_userns, inode->i_sb->s_user_ns,
395 attr->ia_vfsuid))
396 return -EOVERFLOW;
397 if (ia_valid & ATTR_GID &&
398 !vfsgid_has_fsmapping(mnt_userns, inode->i_sb->s_user_ns,
399 attr->ia_vfsgid))
400 return -EOVERFLOW;
401
402 /* Don't allow modifications of files with invalid uids or
403 * gids unless those uids & gids are being made valid.
404 */
405 if (!(ia_valid & ATTR_UID) &&
406 !vfsuid_valid(i_uid_into_vfsuid(mnt_userns, inode)))
407 return -EOVERFLOW;
408 if (!(ia_valid & ATTR_GID) &&
409 !vfsgid_valid(i_gid_into_vfsgid(mnt_userns, inode)))
410 return -EOVERFLOW;
411
412 error = security_inode_setattr(mnt_userns, dentry, attr);
413 if (error)
414 return error;
415 error = try_break_deleg(inode, delegated_inode);
416 if (error)
417 return error;
418
419 if (inode->i_op->setattr)
420 error = inode->i_op->setattr(mnt_userns, dentry, attr);
421 else
422 error = simple_setattr(mnt_userns, dentry, attr);
423
424 if (!error) {
425 fsnotify_change(dentry, ia_valid);
> 426 security_inode_post_setattr(mnt_userns, dentry, ia_valid);
Hi Kees,
I love your patch! Perhaps something to improve:
[auto build test WARNING on kees/for-next/pstore]
[also build test WARNING on pcmoore-audit/next pcmoore-selinux/next linus/master v6.1-rc1 next-20221017]
[cannot apply to zohar-integrity/next-integrity]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/integrity-Move-hooks-into-LSM/20221014-063953
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
patch link: https://lore.kernel.org/r/20221013223654.659758-5-keescook%40chromium.org
patch subject: [PATCH 5/9] LSM: Introduce inode_post_setattr hook
config: x86_64-randconfig-a013
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/4a05d49d9e6e09539a85db353b335221f1181f38
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kees-Cook/integrity-Move-hooks-into-LSM/20221014-063953
git checkout 4a05d49d9e6e09539a85db353b335221f1181f38
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> security/security.c:1336:6: warning: no previous prototype for 'security_inode_post_setattr' [-Wmissing-prototypes]
1336 | void security_inode_post_setattr(struct user_namespace *mnt_userns,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/security_inode_post_setattr +1336 security/security.c
1335
> 1336 void security_inode_post_setattr(struct user_namespace *mnt_userns,
1337 struct dentry *dentry, unsigned int ia_valid)
1338 {
1339 if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
1340 return;
1341 call_void_hook(inode_post_setattr, mnt_userns, dentry, ia_valid);
1342 }
1343
On Thu, Oct 13, 2022 at 03:36:50PM -0700, Kees Cook wrote:
> IMA and EVM need to hook after setattr finishes. Introduce this hook and
> move IMA and EVM's open-coded stacking to use it.
>
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Jonathan McDowell <noodles@fb.com>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> fs/attr.c | 3 +--
> include/linux/evm.h | 6 ------
> include/linux/ima.h | 9 ---------
> include/linux/lsm_hook_defs.h | 3 +++
> security/integrity/evm/evm_main.c | 10 +++++++++-
> security/integrity/ima/ima.h | 2 ++
> security/integrity/ima/ima_appraise.c | 2 +-
> security/integrity/ima/ima_main.c | 1 +
> security/security.c | 8 ++++++++
> 9 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 1552a5f23d6b..e5731057426b 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -423,8 +423,7 @@ int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
>
> if (!error) {
> fsnotify_change(dentry, ia_valid);
> - ima_inode_post_setattr(mnt_userns, dentry);
> - evm_inode_post_setattr(dentry, ia_valid);
> + security_inode_post_setattr(mnt_userns, dentry, ia_valid);
I like that change. In general, no more separate evm_* and ima_*
invocations in the vfs would be much appreciated.
@@ -423,8 +423,7 @@ int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
if (!error) {
fsnotify_change(dentry, ia_valid);
- ima_inode_post_setattr(mnt_userns, dentry);
- evm_inode_post_setattr(dentry, ia_valid);
+ security_inode_post_setattr(mnt_userns, dentry, ia_valid);
}
return error;
@@ -23,7 +23,6 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
struct integrity_iint_cache *iint);
extern int evm_inode_setattr(struct user_namespace *mnt_userns,
struct dentry *dentry, struct iattr *attr);
-extern void evm_inode_post_setattr(struct dentry *dentry, int ia_valid);
extern int evm_inode_setxattr(struct user_namespace *mnt_userns,
struct dentry *dentry, const char *name,
const void *value, size_t size);
@@ -75,11 +74,6 @@ static inline int evm_inode_setattr(struct user_namespace *mnt_userns,
return 0;
}
-static inline void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
-{
- return;
-}
-
static inline int evm_inode_setxattr(struct user_namespace *mnt_userns,
struct dentry *dentry, const char *name,
const void *value, size_t size)
@@ -127,20 +127,11 @@ static inline void ima_post_key_create_or_update(struct key *keyring,
#ifdef CONFIG_IMA_APPRAISE
extern bool is_ima_appraise_enabled(void);
-extern void ima_inode_post_setattr(struct user_namespace *mnt_userns,
- struct dentry *dentry);
#else
static inline bool is_ima_appraise_enabled(void)
{
return 0;
}
-
-static inline void ima_inode_post_setattr(struct user_namespace *mnt_userns,
- struct dentry *dentry)
-{
- return;
-}
-
#endif /* CONFIG_IMA_APPRAISE */
#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
@@ -135,6 +135,9 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
bool rcu)
LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
LSM_HOOK(int, 0, inode_setattr, struct dentry *dentry, struct iattr *attr)
+LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr,
+ struct user_namespace *mnt_userns, struct dentry *dentry,
+ unsigned int ia_valid)
LSM_HOOK(int, 0, inode_getattr, const struct path *path)
LSM_HOOK(int, 0, inode_setxattr, struct user_namespace *mnt_userns,
struct dentry *dentry, const char *name, const void *value,
@@ -817,7 +817,9 @@ int evm_inode_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
* This function is called from notify_change(), which expects the caller
* to lock the inode's i_mutex.
*/
-void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
+static void evm_inode_post_setattr(struct user_namespace *mnt_userns,
+ struct dentry *dentry,
+ unsigned int ia_valid)
{
if (!evm_revalidate_status(NULL))
return;
@@ -905,6 +907,12 @@ static int __init init_evm(void)
late_initcall(init_evm);
+static struct security_hook_list evm_hooks[] __lsm_ro_after_init = {
+ LSM_HOOK_INIT(inode_post_setattr, evm_inode_post_setattr),
+};
+
void __init integrity_lsm_evm_init(void)
{
+ pr_info("Integrity LSM enabling EVM\n");
+ integrity_add_lsm_hooks(evm_hooks, ARRAY_SIZE(evm_hooks));
}
@@ -176,6 +176,8 @@ int ima_inode_setxattr(struct user_namespace *mnt_userns,
int flags);
int ima_inode_removexattr(struct user_namespace *mnt_userns,
struct dentry *dentry, const char *xattr_name);
+void ima_inode_post_setattr(struct user_namespace *mnt_userns,
+ struct dentry *dentry, unsigned int ia_valid);
#endif
/*
@@ -631,7 +631,7 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
* to lock the inode's i_mutex.
*/
void ima_inode_post_setattr(struct user_namespace *mnt_userns,
- struct dentry *dentry)
+ struct dentry *dentry, unsigned int ia_valid)
{
struct inode *inode = d_backing_inode(dentry);
struct integrity_iint_cache *iint;
@@ -1093,6 +1093,7 @@ static struct security_hook_list ima_hooks[] __lsm_ro_after_init = {
#ifdef CONFIG_IMA_APPRAISE
LSM_HOOK_INIT(inode_setxattr, ima_inode_setxattr),
LSM_HOOK_INIT(inode_removexattr, ima_inode_removexattr),
+ LSM_HOOK_INIT(inode_post_setattr, ima_inode_post_setattr),
#endif
};
@@ -1333,6 +1333,14 @@ int security_inode_setattr(struct user_namespace *mnt_userns,
}
EXPORT_SYMBOL_GPL(security_inode_setattr);
+void security_inode_post_setattr(struct user_namespace *mnt_userns,
+ struct dentry *dentry, unsigned int ia_valid)
+{
+ if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+ return;
+ call_void_hook(inode_post_setattr, mnt_userns, dentry, ia_valid);
+}
+
int security_inode_getattr(const struct path *path)
{
if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))