[PATCH] tune2fs: allow remove VERITY

[PATCH] tune2fs: allow remove VERITY

[Gwendal Grignou]

Allow verity flag to be removed from the susperblock:
Tests:
- check the signed file is readable by older kernel after flag
is removed. EXT4_VERITY_FL replaces EXT4_EXT_MIGRATE that has been
removed in 2009.
- when a new kernel is reinstalled, check reenabling verity flag
allow signature to be verified (fsverity measure ...).

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 misc/tune2fs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 314cc0d0..724b8014 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -198,7 +198,8 @@ static __u32 clear_ok_features[3] = {
 		EXT4_FEATURE_RO_COMPAT_QUOTA |
 		EXT4_FEATURE_RO_COMPAT_PROJECT |
 		EXT4_FEATURE_RO_COMPAT_METADATA_CSUM |
-		EXT4_FEATURE_RO_COMPAT_READONLY
+		EXT4_FEATURE_RO_COMPAT_READONLY |
+		EXT4_FEATURE_RO_COMPAT_VERITY
 };
 
 /**
-- 
2.27.0.111.gc72c7da667-goog

Re: [PATCH] tune2fs: allow remove VERITY

[Eric Biggers]

On Tue, Jun 23, 2020 at 07:31:07PM -0700, Gwendal Grignou wrote:
> Allow verity flag to be removed from the susperblock:
> Tests:
> - check the signed file is readable by older kernel after flag
> is removed. EXT4_VERITY_FL replaces EXT4_EXT_MIGRATE that has been
> removed in 2009.
> - when a new kernel is reinstalled, check reenabling verity flag
> allow signature to be verified (fsverity measure ...).
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  misc/tune2fs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 314cc0d0..724b8014 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -198,7 +198,8 @@ static __u32 clear_ok_features[3] = {
>  		EXT4_FEATURE_RO_COMPAT_QUOTA |
>  		EXT4_FEATURE_RO_COMPAT_PROJECT |
>  		EXT4_FEATURE_RO_COMPAT_METADATA_CSUM |
> -		EXT4_FEATURE_RO_COMPAT_READONLY
> +		EXT4_FEATURE_RO_COMPAT_READONLY |
> +		EXT4_FEATURE_RO_COMPAT_VERITY
>  };
>  

tune2fs doesn't allow removing features like encrypt, casefold, verity, extents,
and ea_inode because it doesn't know whether there are any inodes on the
filesystem that are using these features.  These features can't be removed if
there are any inodes using them.

There was recently a suggestion to make tune2fs scan the inode table to
determine whether it is safe to remove these flags; see
https://lkml.kernel.org/linux-ext4/C0761869-5FCD-4CC7-9635-96C18744A0F8@dilger.ca
and
https://lkml.kernel.org/linux-ext4/20200407053213.GC102437@sol.localdomain

I think you'd need to implement that in order for clearing verity to be safe.

Note that misc/tune2fs.8.in would also need to be updated to remove the sentence
that says that clearing the verity feature is unsuppported.

- Eric

Re: [PATCH] tune2fs: allow remove VERITY

[Gwendal Grignou]

On Tue, Jun 23, 2020 at 7:40 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 23, 2020 at 07:31:07PM -0700, Gwendal Grignou wrote:
> > Allow verity flag to be removed from the susperblock:
> > Tests:
> > - check the signed file is readable by older kernel after flag
> > is removed. EXT4_VERITY_FL replaces EXT4_EXT_MIGRATE that has been
> > removed in 2009.
> > - when a new kernel is reinstalled, check reenabling verity flag
> > allow signature to be verified (fsverity measure ...).
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > ---
> >  misc/tune2fs.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > index 314cc0d0..724b8014 100644
> > --- a/misc/tune2fs.c
> > +++ b/misc/tune2fs.c
> > @@ -198,7 +198,8 @@ static __u32 clear_ok_features[3] = {
> >               EXT4_FEATURE_RO_COMPAT_QUOTA |
> >               EXT4_FEATURE_RO_COMPAT_PROJECT |
> >               EXT4_FEATURE_RO_COMPAT_METADATA_CSUM |
> > -             EXT4_FEATURE_RO_COMPAT_READONLY
> > +             EXT4_FEATURE_RO_COMPAT_READONLY |
> > +             EXT4_FEATURE_RO_COMPAT_VERITY
> >  };
> >
>
> tune2fs doesn't allow removing features like encrypt, casefold, verity, extents,
> and ea_inode because it doesn't know whether there are any inodes on the
> filesystem that are using these features.  These features can't be removed if
> there are any inodes using them.
The verity case is slightly different though: beside metadata,
encrypted files are useless.
In the case of fs-verity, the file is still readable, its size is
correct. Using debugfs, I checked the merkel tree blocks appended at
the end of the file are still mapped to the file inode, they are
marked as free when the file is removed.
Are you concerned about filesystem corruption? When I re-enable the
features and load a kernel that supports fs-verity, the protected
files are signed and read-only as expected.

>
> There was recently a suggestion to make tune2fs scan the inode table to
> determine whether it is safe to remove these flags; see
> https://lkml.kernel.org/linux-ext4/C0761869-5FCD-4CC7-9635-96C18744A0F8@dilger.ca
> and
> https://lkml.kernel.org/linux-ext4/20200407053213.GC102437@sol.localdomain
The intent is to be able to rollback to an older kernel. Given the
file system will contain files protected by fs-verity, tune2fs will
not be able to remove the feature.
>
> I think you'd need to implement that in order for clearing verity to be safe.
>
> Note that misc/tune2fs.8.in would also need to be updated to remove the sentence
> that says that clearing the verity feature is unsuppported.
Will do.

Gwendal.
>
> - Eric

Re: [PATCH] tune2fs: allow remove VERITY

[Eric Biggers]

On Wed, Jun 24, 2020 at 12:06:22AM -0700, Gwendal Grignou wrote:
> On Tue, Jun 23, 2020 at 7:40 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Tue, Jun 23, 2020 at 07:31:07PM -0700, Gwendal Grignou wrote:
> > > Allow verity flag to be removed from the susperblock:
> > > Tests:
> > > - check the signed file is readable by older kernel after flag
> > > is removed. EXT4_VERITY_FL replaces EXT4_EXT_MIGRATE that has been
> > > removed in 2009.
> > > - when a new kernel is reinstalled, check reenabling verity flag
> > > allow signature to be verified (fsverity measure ...).
> > >
> > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > > ---
> > >  misc/tune2fs.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > > index 314cc0d0..724b8014 100644
> > > --- a/misc/tune2fs.c
> > > +++ b/misc/tune2fs.c
> > > @@ -198,7 +198,8 @@ static __u32 clear_ok_features[3] = {
> > >               EXT4_FEATURE_RO_COMPAT_QUOTA |
> > >               EXT4_FEATURE_RO_COMPAT_PROJECT |
> > >               EXT4_FEATURE_RO_COMPAT_METADATA_CSUM |
> > > -             EXT4_FEATURE_RO_COMPAT_READONLY
> > > +             EXT4_FEATURE_RO_COMPAT_READONLY |
> > > +             EXT4_FEATURE_RO_COMPAT_VERITY
> > >  };
> > >
> >
> > tune2fs doesn't allow removing features like encrypt, casefold, verity, extents,
> > and ea_inode because it doesn't know whether there are any inodes on the
> > filesystem that are using these features.  These features can't be removed if
> > there are any inodes using them.
>
> The verity case is slightly different though: beside metadata,
> encrypted files are useless.
> In the case of fs-verity, the file is still readable, its size is
> correct. Using debugfs, I checked the merkel tree blocks appended at
> the end of the file are still mapped to the file inode, they are
> marked as free when the file is removed.
> Are you concerned about filesystem corruption? When I re-enable the
> features and load a kernel that supports fs-verity, the protected
> files are signed and read-only as expected.

The problem is that the old, non-verity-aware kernel will allow writing to
verity files, because it ignores the verity inode flag.  That will get files'
data out of sync with their Merkle trees, and possibly corrupt their Merkle
trees.

This is why verity is a RO_COMPAT feature rather than a COMPAT one.  This
ensures that if the kernel isn't aware of verity, then the filesystem can only
be mounted read-only.

How about making 'tune2fs -O ^verity' remove the verity flag from all files
that have it and remove their Merkle trees by truncating blocks past i_size?
Would that work for you, or do you really need the verity-ness of files to be
preserved over 'tune2fs -O ^verity; tune2fs -O verity'?

- Eric

Re: [PATCH] tune2fs: allow remove VERITY

[Gwendal Grignou]

On Wed, Jun 24, 2020 at 9:09 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Jun 24, 2020 at 12:06:22AM -0700, Gwendal Grignou wrote:
> > On Tue, Jun 23, 2020 at 7:40 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Tue, Jun 23, 2020 at 07:31:07PM -0700, Gwendal Grignou wrote:
> > > > Allow verity flag to be removed from the susperblock:
> > > > Tests:
> > > > - check the signed file is readable by older kernel after flag
> > > > is removed. EXT4_VERITY_FL replaces EXT4_EXT_MIGRATE that has been
> > > > removed in 2009.
> > > > - when a new kernel is reinstalled, check reenabling verity flag
> > > > allow signature to be verified (fsverity measure ...).
> > > >
> > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > > > ---
> > > >  misc/tune2fs.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > > > index 314cc0d0..724b8014 100644
> > > > --- a/misc/tune2fs.c
> > > > +++ b/misc/tune2fs.c
> > > > @@ -198,7 +198,8 @@ static __u32 clear_ok_features[3] = {
> > > >               EXT4_FEATURE_RO_COMPAT_QUOTA |
> > > >               EXT4_FEATURE_RO_COMPAT_PROJECT |
> > > >               EXT4_FEATURE_RO_COMPAT_METADATA_CSUM |
> > > > -             EXT4_FEATURE_RO_COMPAT_READONLY
> > > > +             EXT4_FEATURE_RO_COMPAT_READONLY |
> > > > +             EXT4_FEATURE_RO_COMPAT_VERITY
> > > >  };
> > > >
> > >
> > > tune2fs doesn't allow removing features like encrypt, casefold, verity, extents,
> > > and ea_inode because it doesn't know whether there are any inodes on the
> > > filesystem that are using these features.  These features can't be removed if
> > > there are any inodes using them.
> >
> > The verity case is slightly different though: beside metadata,
> > encrypted files are useless.
> > In the case of fs-verity, the file is still readable, its size is
> > correct. Using debugfs, I checked the merkel tree blocks appended at
> > the end of the file are still mapped to the file inode, they are
> > marked as free when the file is removed.
> > Are you concerned about filesystem corruption? When I re-enable the
> > features and load a kernel that supports fs-verity, the protected
> > files are signed and read-only as expected.
>
> The problem is that the old, non-verity-aware kernel will allow writing to
> verity files, because it ignores the verity inode flag.  That will get files'
> data out of sync with their Merkle trees, and possibly corrupt their Merkle
> trees.
You're right, an older kernel modifying the verity file corrupts the
merkel tree and creates inconsistency between the blocks where the
merkel tree was and the inode information.
We could mark the file (not just the inode) as read only in
fsverity_ioctl_enable() but that's a layering violation.
>
> This is why verity is a RO_COMPAT feature rather than a COMPAT one.  This
> ensures that if the kernel isn't aware of verity, then the filesystem can only
> be mounted read-only.
It is indeed safe, but having the whole filesystem read-only is very
restrictive.

>
> How about making 'tune2fs -O ^verity' remove the verity flag from all files
> that have it and remove their Merkle trees by truncating blocks past i_size?
> Would that work for you, or do you really need the verity-ness of files to be
> preserved over 'tune2fs -O ^verity; tune2fs -O verity'?
The latter would be better. Another use case would be a filesystem on
an external device, shared by machines with different kernels. Even if
there are no fs-verity files, the older kernel can not mount it
read-write.
Removing the verity feature would break the trust in the filesystem
when the device is plugged in the original machine.

As long as the verified files are not extended (remove is fine), the
option flag can be removed. No solutions are satisfactory, but for
security sake, the current behavior is the safest.

Gwendal.

>
> - Eric

Re: [PATCH] tune2fs: allow remove VERITY

[Eric Biggers]

On Thu, Jun 25, 2020 at 11:26:44AM -0700, Gwendal Grignou wrote:
> On Wed, Jun 24, 2020 at 9:09 AM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Wed, Jun 24, 2020 at 12:06:22AM -0700, Gwendal Grignou wrote:
> > > On Tue, Jun 23, 2020 at 7:40 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > On Tue, Jun 23, 2020 at 07:31:07PM -0700, Gwendal Grignou wrote:
> > > > > Allow verity flag to be removed from the susperblock:
> > > > > Tests:
> > > > > - check the signed file is readable by older kernel after flag
> > > > > is removed. EXT4_VERITY_FL replaces EXT4_EXT_MIGRATE that has been
> > > > > removed in 2009.
> > > > > - when a new kernel is reinstalled, check reenabling verity flag
> > > > > allow signature to be verified (fsverity measure ...).
> > > > >
> > > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > ---
> > > > >  misc/tune2fs.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > > > > index 314cc0d0..724b8014 100644
> > > > > --- a/misc/tune2fs.c
> > > > > +++ b/misc/tune2fs.c
> > > > > @@ -198,7 +198,8 @@ static __u32 clear_ok_features[3] = {
> > > > >               EXT4_FEATURE_RO_COMPAT_QUOTA |
> > > > >               EXT4_FEATURE_RO_COMPAT_PROJECT |
> > > > >               EXT4_FEATURE_RO_COMPAT_METADATA_CSUM |
> > > > > -             EXT4_FEATURE_RO_COMPAT_READONLY
> > > > > +             EXT4_FEATURE_RO_COMPAT_READONLY |
> > > > > +             EXT4_FEATURE_RO_COMPAT_VERITY
> > > > >  };
> > > > >
> > > >
> > > > tune2fs doesn't allow removing features like encrypt, casefold, verity, extents,
> > > > and ea_inode because it doesn't know whether there are any inodes on the
> > > > filesystem that are using these features.  These features can't be removed if
> > > > there are any inodes using them.
> > >
> > > The verity case is slightly different though: beside metadata,
> > > encrypted files are useless.
> > > In the case of fs-verity, the file is still readable, its size is
> > > correct. Using debugfs, I checked the merkel tree blocks appended at
> > > the end of the file are still mapped to the file inode, they are
> > > marked as free when the file is removed.
> > > Are you concerned about filesystem corruption? When I re-enable the
> > > features and load a kernel that supports fs-verity, the protected
> > > files are signed and read-only as expected.
> >
> > The problem is that the old, non-verity-aware kernel will allow writing to
> > verity files, because it ignores the verity inode flag.  That will get files'
> > data out of sync with their Merkle trees, and possibly corrupt their Merkle
> > trees.
> You're right, an older kernel modifying the verity file corrupts the
> merkel tree and creates inconsistency between the blocks where the
> merkel tree was and the inode information.
> We could mark the file (not just the inode) as read only in
> fsverity_ioctl_enable() but that's a layering violation.

Even if we had enforced that verity files never have any writable mode bits set
(mode & 0222), it wouldn't matter because old kernels could just chmod() the
file back to writable.  Similarly for the immutable flag.

> >
> > This is why verity is a RO_COMPAT feature rather than a COMPAT one.  This
> > ensures that if the kernel isn't aware of verity, then the filesystem can only
> > be mounted read-only.
> It is indeed safe, but having the whole filesystem read-only is very
> restrictive.

Unfortunately there's no concept of ro_compat-ness for ext4 inode flags.
If there was, we would have used that instead.

Only the filesystem feature flags have the notion of compat/ro_compat/incompat.

> >
> > How about making 'tune2fs -O ^verity' remove the verity flag from all files
> > that have it and remove their Merkle trees by truncating blocks past i_size?
> > Would that work for you, or do you really need the verity-ness of files to be
> > preserved over 'tune2fs -O ^verity; tune2fs -O verity'?
> The latter would be better. Another use case would be a filesystem on
> an external device, shared by machines with different kernels. Even if
> there are no fs-verity files, the older kernel can not mount it
> read-write.
> Removing the verity feature would break the trust in the filesystem
> when the device is plugged in the original machine.
> 
> As long as the verified files are not extended (remove is fine), the
> option flag can be removed. No solutions are satisfactory, but for
> security sake, the current behavior is the safest.

I just don't think verity files existing with the verity feature disabled can be
a supported filesystem state, for the reasons I mentioned.

Can't you just re-enable verity on the files when re-enabling the feature flag?
Userspace must know which files are supposed to have verity enabled anyway.

- Eric