| Message ID | 20250119145941.22094-2-James.Bottomley@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | efivarfs: fix ability to mimic uncommitted variables | expand |
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? Should we complain about this instead?
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
On Sun, 19 Jan 2025 at 17:48, James Bottomley <James.Bottomley@hansenpartnership.com> 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@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. > Fair enough I'll queue these up - thanks.
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, };
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(+)