Re: [PATCH v4 28/30] audit,landlock: Add AUDIT_EXE_LANDLOCK_DENY rule type
From: Jann Horn
Date: Mon Jan 13 2025 - 10:04:31 EST
+Christian and Al Viro to double-check what I'm saying
On Wed, Jan 8, 2025 at 4:44 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> -static const void *get_current_exe(const char **path_str, size_t *path_size)
> +static const void *get_current_exe(const char **path_str, size_t *path_size,
> + struct inode **inode)
> {
> struct mm_struct *mm = current->mm;
> struct file *file __free(fput) = NULL;
> @@ -93,6 +96,8 @@ static const void *get_current_exe(const char **path_str, size_t *path_size)
>
> *path_size = size;
> *path_str = path;
> + ihold(file_inode(file));
> + *inode = file_inode(file);
> return no_free_ptr(buffer);
> }
This looks unsafe: Once the reference to the file has been dropped
(which happens implicitly on return from get_current_exe()), nothing
holds a reference on the mount point or superblock anymore (the file
was previously holding a reference to the mount point through
->f_path.mnt), and so the superblock can be torn down and freed. But
the reference to the inode lives longer and is only cleaned up on
return from the caller get_current_details().
So I think this code can hit the error check for "Busy inodes after
unmount" in generic_shutdown_super(), which indicates that in theory,
use-after-free can occur.
For context, here are two older kernel security issues that also
involved superblock UAF due to assuming that it's possible to just
hold refcounted references to inodes:
https://project-zero.issues.chromium.org/42451116
https://project-zero.issues.chromium.org/379667898
For fixing this, one option would be to copy the entire "struct path"
(which holds references on both the mount point and the inode) instead
of just copying the inode pointer.