On Sun, 2025-01-19 at 17:32 +0100, Ard Biesheuvel wrote:
> On Sun, 19 Jan 2025 at 16:00, James Bottomley
> <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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
