Re: [PATCH 1/2] efivarfs: prevent setting of zero size on the inodes in the cache

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Sun, 2025-01-19 at 17:32 +0100, Ard Biesheuvel wrote:
> On Sun, 19 Jan 2025 at 16:00, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > Current efivarfs uses simple_setattr which allows the setting of
> > any
> > size in the inode cache.  This is wrong because a zero size file is
> > used to indicate an "uncommitted" variable, so by simple means of
> > truncating the file (as root) any variable may be turned to look
> > like
> > it's uncommitted.  Fix by adding an efivarfs_setattr routine which
> > does not allow updating of the cached inode size (which now only
> > comes
> > from the underlying variable).
> > 
> > Signed-off-by: James Bottomley
> > <James.Bottomley@HansenPartnership.com>
> > ---
> >  fs/efivarfs/inode.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
> > index ec23da8405ff..a4a6587ecd2e 100644
> > --- a/fs/efivarfs/inode.c
> > +++ b/fs/efivarfs/inode.c
> > @@ -187,7 +187,24 @@ efivarfs_fileattr_set(struct mnt_idmap *idmap,
> >         return 0;
> >  }
> > 
> > +/* copy of simple_setattr except that it doesn't do i_size updates
> > */
> > +static int efivarfs_setattr(struct mnt_idmap *idmap, struct dentry
> > *dentry,
> > +                  struct iattr *iattr)
> > +{
> > +       struct inode *inode = d_inode(dentry);
> > +       int error;
> > +
> > +       error = setattr_prepare(idmap, dentry, iattr);
> > +       if (error)
> > +               return error;
> > +
> > +       setattr_copy(idmap, inode, iattr);
> > +       mark_inode_dirty(inode);
> > +       return 0;
> > +}
> > +
> >  static const struct inode_operations
> > efivarfs_file_inode_operations = {
> >         .fileattr_get = efivarfs_fileattr_get,
> >         .fileattr_set = efivarfs_fileattr_set,
> > +       .setattr      = efivarfs_setattr,
> >  };
> 
> Is it sufficient to just ignore inode size changes?

Yes, as far as my testing goes.

>  Should we complain about this instead?

I don't think so because every variable write (at least from the shell)
tends to start off with a truncation so we'd get a lot of spurious
complaints.

Regards,

James