Skip to contentSkip to conversation

bpf: fs/xattr: don't assume the inode is locked in path_unlink/path_rmdir

PATCH Applied Andrea Parri 3 participants 4 messages Sep 22 – Sep 25

Subsystems (3)
Stable branch VFS BPF
lore
Share & tools
lore

Andrea Parri

author
#0 lore raw

PATCH 1/1 bpf: fs/xattr: don't assume the inode is locked in path_unlink/path_rmdir

bpf_lsm_has_d_inode_locked() makes the verifier rewrite bpf_[set|remove]_dentry_xattr() to the _locked variants, which assume that the caller already holds the inode's i_rwsem. The path_unlink and path_rmdir hooks are listed, but security_path_unlink() and security_path_rmdir() run before vfs_unlink()/vfs_rmdir() take the victim inode's i_rwsem, so a sleepable BPF LSM program attached to either hook mutates the victim's xattrs without the lock held.

Drop the two path hooks from d_inode_locked_hooks so that the verifier keeps the locking bpf_[set|remove]_dentry_xattr() variants, which take the lock themselves.

Fixes 56467292794b8 ("bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs")
Cc [email protected]
Assisted-by LLM
Signed-off-by Andrea Parri <[email protected]>
---
 fs/bpf_fs_kfuncs.c | 4 ----
 1 file changed, 4 deletions(-)
fs/bpf_fs_kfuncs.c +0 -4
index 6cb8772679782..357a379ef92a5 100644
@@ -472,10 +472,6 @@ BTF_ID(func, bpf_lsm_inode_rmdir)
 BTF_ID(func, bpf_lsm_inode_setattr)
 BTF_ID(func, bpf_lsm_inode_setxattr)
 BTF_ID(func, bpf_lsm_inode_unlink)
-#ifdef CONFIG_SECURITY_PATH
-BTF_ID(func, bpf_lsm_path_unlink)
-BTF_ID(func, bpf_lsm_path_rmdir)
-#endif /* CONFIG_SECURITY_PATH */
 BTF_SET_END(d_inode_locked_hooks)
 
 bool bpf_lsm_has_d_inode_locked(const struct bpf_prog *prog)
Signature · 1 line
2.53.0
#1 lore raw
Quoted 25 lines · Andrea Parri

bpf_lsm_has_d_inode_locked() makes the verifier rewrite bpf_[set|remove]_dentry_xattr() to the _locked variants, which assume that the caller already holds the inode's i_rwsem. The path_unlink and path_rmdir hooks are listed, but security_path_unlink() and security_path_rmdir() run before vfs_unlink()/vfs_rmdir() take the victim inode's i_rwsem, so a sleepable BPF LSM program attached to either hook mutates the victim's xattrs without the lock held.

Drop the two path hooks from d_inode_locked_hooks so that the verifier keeps the locking bpf_[set|remove]_dentry_xattr() variants, which take the lock themselves.

Fixes 56467292794b8 ("bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs")
Cc [email protected]
Assisted-by LLM
Signed-off-by Andrea Parri <[email protected]>
---
 fs/bpf_fs_kfuncs.c | 4 ----
 1 file changed, 4 deletions(-)
fs/bpf_fs_kfuncs.c +0 -0
index 6cb8772679782..357a379ef92a5 100644

fs/bpf_fs_kfuncs.c BTF_ID(func, bpf_lsm_inode_rmdir) · line 475

Quoted 8 lines
fs/bpf_fs_kfuncs.c +0 -4
@@ -472,10 +472,6 @@ BTF_ID(func, bpf_lsm_inode_rmdir)
 BTF_ID(func, bpf_lsm_inode_setattr)
 BTF_ID(func, bpf_lsm_inode_setxattr)
 BTF_ID(func, bpf_lsm_inode_unlink)
-#ifdef CONFIG_SECURITY_PATH
-BTF_ID(func, bpf_lsm_path_unlink)
-BTF_ID(func, bpf_lsm_path_rmdir)
-#endif /* CONFIG_SECURITY_PATH */
 BTF_SET_END(d_inode_locked_hooks)

Christian,
this looks correct. Want to take it via vfs?

maintainer
#2 lore raw
Quoted 21 lines · Alexei Starovoitov
Quoted 20 lines · Andrea Parri

bpf_lsm_has_d_inode_locked() makes the verifier rewrite bpf_[set|remove]_dentry_xattr() to the _locked variants, which assume that the caller already holds the inode's i_rwsem. The path_unlink and path_rmdir hooks are listed, but security_path_unlink() and security_path_rmdir() run before vfs_unlink()/vfs_rmdir() take the victim inode's i_rwsem, so a sleepable BPF LSM program attached to either hook mutates the victim's xattrs without the lock held.

Drop the two path hooks from d_inode_locked_hooks so that the verifier keeps the locking bpf_[set|remove]_dentry_xattr() variants, which take the lock themselves.

Fixes 56467292794b8 ("bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs")
Cc [email protected]
Assisted-by LLM
Signed-off-by Andrea Parri <[email protected]>
---
 fs/bpf_fs_kfuncs.c | 4 ----
 1 file changed, 4 deletions(-)
Quoted 16 lines

fs/bpf_fs_kfuncs.c BTF_ID(func, bpf_lsm_inode_rmdir) · line 475

Quoted 13 lines
fs/bpf_fs_kfuncs.c +0 -4
index 6cb8772679782..357a379ef92a5 100644
@@ -472,10 +472,6 @@ BTF_ID(func, bpf_lsm_inode_rmdir)
 BTF_ID(func, bpf_lsm_inode_setattr)
 BTF_ID(func, bpf_lsm_inode_setxattr)
 BTF_ID(func, bpf_lsm_inode_unlink)
-#ifdef CONFIG_SECURITY_PATH
-BTF_ID(func, bpf_lsm_path_unlink)
-BTF_ID(func, bpf_lsm_path_rmdir)
-#endif /* CONFIG_SECURITY_PATH */
 BTF_SET_END(d_inode_locked_hooks)

Christian,
this looks correct. Want to take it via vfs?

Yes, picked up now. Sorry, foot injury was afk.

maintainer Applied
#3 lore raw
In reply to Andrea Parri
Quoted 1 line · Andrea Parri

bpf: fs/xattr: don't assume the inode is locked in path_unlink/path_rmdir

Applied to the vfs.fixes branch of the vfs/vfs.git tree. Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] bpf: fs/xattr: don't assume the inode is locked in path_unlink/path_rmdir

      https://git.kernel.org/vfs/vfs/c/35d442ed1f86