| Message ID | 20250407-work-anon_inode-v1-0-53a44c20d44e@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | fs: harden anon inodes | expand |
On Mon, Apr 7, 2025 at 11:54 AM Christian Brauner <brauner@kernel.org> wrote: > > * Anonymous inodes currently don't come with a proper mode causing > issues in the kernel when we want to add useful VFS debug assert. Fix > that by giving them a proper mode and masking it off when we report it > to userspace which relies on them not having any mode. > > * Anonymous inodes currently allow to change inode attributes because > the VFS falls back to simple_setattr() if i_op->setattr isn't > implemented. This means the ownership and mode for every single user > of anon_inode_inode can be changed. Block that as it's either useless > or actively harmful. If specific ownership is needed the respective > subsystem should allocate anonymous inodes from their own private > superblock. > > * Port pidfs to the new anon_inode_{g,s}etattr() helpers. > > * Add proper tests for anonymous inode behavior. > > The anonymous inode specific fixes should ideally be backported to all > LTS kernels. > > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > Christian Brauner (9): > anon_inode: use a proper mode internally > pidfs: use anon_inode_getattr() > anon_inode: explicitly block ->setattr() > pidfs: use anon_inode_setattr() > anon_inode: raise SB_I_NODEV and SB_I_NOEXEC > selftests/filesystems: add first test for anonymous inodes > selftests/filesystems: add second test for anonymous inodes > selftests/filesystems: add third test for anonymous inodes > selftests/filesystems: add fourth test for anonymous inodes > I have two nits, past that LGTM 1. I would add a comment explaining why S_IFREG in alloc_anon_inode() 2. commit messages for selftests could spell out what's being added instead of being counted, it's all one-liners for example: selftests/filesystems: validate that anonymous inodes cannot be chown()ed
On Mon, 2025-04-07 at 11:54 +0200, Christian Brauner wrote: > * Anonymous inodes currently don't come with a proper mode causing > issues in the kernel when we want to add useful VFS debug assert. Fix > that by giving them a proper mode and masking it off when we report it > to userspace which relies on them not having any mode. > > * Anonymous inodes currently allow to change inode attributes because > the VFS falls back to simple_setattr() if i_op->setattr isn't > implemented. This means the ownership and mode for every single user > of anon_inode_inode can be changed. Block that as it's either useless > or actively harmful. If specific ownership is needed the respective > subsystem should allocate anonymous inodes from their own private > superblock. > > * Port pidfs to the new anon_inode_{g,s}etattr() helpers. > > * Add proper tests for anonymous inode behavior. > > The anonymous inode specific fixes should ideally be backported to all > LTS kernels. > > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > Christian Brauner (9): > anon_inode: use a proper mode internally > pidfs: use anon_inode_getattr() > anon_inode: explicitly block ->setattr() > pidfs: use anon_inode_setattr() > anon_inode: raise SB_I_NODEV and SB_I_NOEXEC > selftests/filesystems: add first test for anonymous inodes > selftests/filesystems: add second test for anonymous inodes > selftests/filesystems: add third test for anonymous inodes > selftests/filesystems: add fourth test for anonymous inodes > > fs/anon_inodes.c | 45 ++++++++++++++ > fs/internal.h | 5 ++ > fs/libfs.c | 2 +- > fs/pidfs.c | 26 +------- > tools/testing/selftests/filesystems/.gitignore | 1 + > tools/testing/selftests/filesystems/Makefile | 2 +- > .../selftests/filesystems/anon_inode_test.c | 69 ++++++++++++++++++++++ > 7 files changed, 124 insertions(+), 26 deletions(-) > --- > base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 > change-id: 20250407-work-anon_inode-e22bb1a74992 > This all looks like good changes to make. I'm still a little curious about what might be dependent on not seeing a file type in st_mode, but if we haven't traditionally reported one, it's probably safest to continue without doing so. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Mon, Apr 07, 2025 at 12:19:45PM +0200, Mateusz Guzik wrote: > On Mon, Apr 7, 2025 at 11:54 AM Christian Brauner <brauner@kernel.org> wrote: > > > > * Anonymous inodes currently don't come with a proper mode causing > > issues in the kernel when we want to add useful VFS debug assert. Fix > > that by giving them a proper mode and masking it off when we report it > > to userspace which relies on them not having any mode. > > > > * Anonymous inodes currently allow to change inode attributes because > > the VFS falls back to simple_setattr() if i_op->setattr isn't > > implemented. This means the ownership and mode for every single user > > of anon_inode_inode can be changed. Block that as it's either useless > > or actively harmful. If specific ownership is needed the respective > > subsystem should allocate anonymous inodes from their own private > > superblock. > > > > * Port pidfs to the new anon_inode_{g,s}etattr() helpers. > > > > * Add proper tests for anonymous inode behavior. > > > > The anonymous inode specific fixes should ideally be backported to all > > LTS kernels. > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > --- > > Christian Brauner (9): > > anon_inode: use a proper mode internally > > pidfs: use anon_inode_getattr() > > anon_inode: explicitly block ->setattr() > > pidfs: use anon_inode_setattr() > > anon_inode: raise SB_I_NODEV and SB_I_NOEXEC > > selftests/filesystems: add first test for anonymous inodes > > selftests/filesystems: add second test for anonymous inodes > > selftests/filesystems: add third test for anonymous inodes > > selftests/filesystems: add fourth test for anonymous inodes > > > > I have two nits, past that LGTM > > 1. I would add a comment explaining why S_IFREG in alloc_anon_inode() /* * Historically anonymous inodes didn't have a type at all and * userspace has come to rely on this. Internally they're just * regular files but S_IFREG is masked off when reporting * information to userspace. */ > 2. commit messages for selftests could spell out what's being added > instead of being counted, it's all one-liners I've replaced the count with chown(), chmod(), exec(), and open(). Thanks!
* Anonymous inodes currently don't come with a proper mode causing issues in the kernel when we want to add useful VFS debug assert. Fix that by giving them a proper mode and masking it off when we report it to userspace which relies on them not having any mode. * Anonymous inodes currently allow to change inode attributes because the VFS falls back to simple_setattr() if i_op->setattr isn't implemented. This means the ownership and mode for every single user of anon_inode_inode can be changed. Block that as it's either useless or actively harmful. If specific ownership is needed the respective subsystem should allocate anonymous inodes from their own private superblock. * Port pidfs to the new anon_inode_{g,s}etattr() helpers. * Add proper tests for anonymous inode behavior. The anonymous inode specific fixes should ideally be backported to all LTS kernels. Signed-off-by: Christian Brauner <brauner@kernel.org> --- Christian Brauner (9): anon_inode: use a proper mode internally pidfs: use anon_inode_getattr() anon_inode: explicitly block ->setattr() pidfs: use anon_inode_setattr() anon_inode: raise SB_I_NODEV and SB_I_NOEXEC selftests/filesystems: add first test for anonymous inodes selftests/filesystems: add second test for anonymous inodes selftests/filesystems: add third test for anonymous inodes selftests/filesystems: add fourth test for anonymous inodes fs/anon_inodes.c | 45 ++++++++++++++ fs/internal.h | 5 ++ fs/libfs.c | 2 +- fs/pidfs.c | 26 +------- tools/testing/selftests/filesystems/.gitignore | 1 + tools/testing/selftests/filesystems/Makefile | 2 +- .../selftests/filesystems/anon_inode_test.c | 69 ++++++++++++++++++++++ 7 files changed, 124 insertions(+), 26 deletions(-) --- base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 change-id: 20250407-work-anon_inode-e22bb1a74992