Re: [PATCH v4 28/30] audit,landlock: Add AUDIT_EXE_LANDLOCK_DENY rule type

["Mickaël Salaün" <mic@digikod.net>]

On Mon, Jan 13, 2025 at 03:55:42PM +0100, Jann Horn wrote:
> +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@digikod.net> 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

Thanks for the detailed explanation!

> 
> 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.

Yes, I'll do that.