[PATCH 0/2] efivarfs: fix ability to mimic uncommitted variables

[PATCH 0/2] efivarfs: fix ability to mimic uncommitted variables

[James Bottomley]

The use of simple_setattr in efivarfs means that anyone who can write
to the variable (which is usually only root) can set the cached inode
size to an arbitrary value (including truncating it to zero).  This
value, while not transmitted on to the underlying variable, does show
up on stat and means that anyone who can write to the variable file
can also make any variable mimic an uncommitted one (a variable with
zero size) which is checked by certain programmes that use EFI
variables, like systemd.  This problem can be fixed by not allowing
anything except a successful variable update to change the inode size.

I also added a regression test to make sure the problem behaviour
isn't reintroduced.

James

---

James Bottomley (2):
  efivarfs: prevent setting of zero size on the inodes in the cache
  selftests/efivarfs: add check for disallowing file truncation

 fs/efivarfs/inode.c                          | 17 +++++++++++++++
 tools/testing/selftests/efivarfs/efivarfs.sh | 23 ++++++++++++++++++++
 2 files changed, 40 insertions(+)

-- 
2.35.3

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

[James Bottomley]

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,
 };
-- 
2.35.3

[PATCH 2/2] selftests/efivarfs: add check for disallowing file truncation

[James Bottomley]

Now that the ability of arbitrary writes to set the inode size is
fixed, verify that a variable file accepts a truncation operation but
does not change the stat size because of it.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 tools/testing/selftests/efivarfs/efivarfs.sh | 23 ++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/efivarfs/efivarfs.sh b/tools/testing/selftests/efivarfs/efivarfs.sh
index d374878cc0ba..96677282789b 100755
--- a/tools/testing/selftests/efivarfs/efivarfs.sh
+++ b/tools/testing/selftests/efivarfs/efivarfs.sh
@@ -202,6 +202,28 @@ test_invalid_filenames()
 	exit $ret
 }
 
+test_no_set_size()
+{
+	local attrs='\x07\x00\x00\x00'
+	local file=$efivarfs_mount/$FUNCNAME-$test_guid
+	local ret=0
+
+	printf "$attrs\x00" > $file
+	[ -e $file -a -s $file ] || exit 1
+	chattr -i $file
+	: > $file
+	if [ $? != 0 ]; then
+		echo "variable file failed to accept truncation"
+		ret=1
+	elif [ -e $file -a ! -s $file ]; then
+		echo "file can be truncated to zero size"
+		ret=1
+	fi
+	rm $file || exit 1
+
+	exit $ret
+}
+
 check_prereqs
 
 rc=0
@@ -214,5 +236,6 @@ run_test test_zero_size_delete
 run_test test_open_unlink
 run_test test_valid_filenames
 run_test test_invalid_filenames
+run_test test_no_set_size
 
 exit $rc
-- 
2.35.3

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

[Ard Biesheuvel]

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?

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

[James Bottomley]

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

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

[Ard Biesheuvel]

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.