On Sun, 19 Jan 2025 at 17:48, James Bottomley
<James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> 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.
>
Fair enough
I'll queue these up - thanks.
