[RFC,v1,1/2] mm/memfd: Add support for F_SEAL_FUTURE_EXEC to memfd

Commit Message

Isaac J. Manjarres Dec. 6, 2024, 1:09 a.m. UTC

Android currently uses the ashmem driver [1] for creating shared memory
regions between processes. Ashmem buffers can initially be mapped with
PROT_READ, PROT_WRITE, and PROT_EXEC. Processes can then use the
ASHMEM_SET_PROT_MASK ioctl command to restrict--never add--the
permissions that the buffer can be mapped with.

Processes can remove the ability to map ashmem buffers as executable to
ensure that those buffers cannot be exploited to run unintended code.
We are currently trying to replace ashmem with memfd. However, memfd
does not have a provision to permanently remove the ability to map a
buffer as executable. Although, this should be something that can be
achieved via a new file seal.

There are known usecases (e.g. CursorWindow [2]) where a process
maps a buffer with read/write permissions before restricting the buffer
to being mapped as read-only for future mappings.

The resulting VMA from the writable mapping has VM_MAYEXEC set, meaning
that mprotect() can change the mapping to be executable. Therefore,
implementing the seal similar to F_SEAL_WRITE would not be appropriate,
since it would not work with the CursorWindow usecase. This is because
the CursorWindow process restricts the mapping permissions to read-only
after the writable mapping is created. So, adding a file seal for
executable mappings that operates like F_SEAL_WRITE would fail.

Therefore, add support for F_SEAL_FUTURE_EXEC, which is handled
similarly to F_SEAL_FUTURE_WRITE. This ensures that CursorWindow can
continue to create a writable mapping initially, and then restrict the
permissions on the buffer to be mappable as read-only by using both
F_SEAL_FUTURE_WRITE and F_SEAL_FUTURE_EXEC. After the seal is
applied, any calls to mmap() with PROT_EXEC will fail.

[1] https://cs.android.com/android/kernel/superproject/+/common-android-mainline:common/drivers/staging/android/ashmem.c
[2] https://developer.android.com/reference/android/database/CursorWindow

Cc: Suren Baghdasaryan <[e-mail]>
Cc: Kalesh Singh <[e-mail]>
Cc: John Stultz <[e-mail]>
Signed-off-by: Isaac J. Manjarres <[e-mail]>
---
 include/linux/mm.h         |  5 +++++
 include/uapi/linux/fcntl.h |  1 +
 mm/memfd.c                 |  1 +
 mm/mmap.c                  | 11 +++++++++++
 4 files changed, 18 insertions(+)

Comments

Kalesh Singh Dec. 6, 2024, 5:49 p.m. UTC | #1

On Thu, Dec 5, 2024 at 5:09 PM Isaac J. Manjarres
<[e-mail]> wrote:
>
> Android currently uses the ashmem driver [1] for creating shared memory
> regions between processes. Ashmem buffers can initially be mapped with
> PROT_READ, PROT_WRITE, and PROT_EXEC. Processes can then use the
> ASHMEM_SET_PROT_MASK ioctl command to restrict--never add--the
> permissions that the buffer can be mapped with.
>
> Processes can remove the ability to map ashmem buffers as executable to
> ensure that those buffers cannot be exploited to run unintended code.
> We are currently trying to replace ashmem with memfd. However, memfd
> does not have a provision to permanently remove the ability to map a
> buffer as executable. Although, this should be something that can be
> achieved via a new file seal.
>
> There are known usecases (e.g. CursorWindow [2]) where a process
> maps a buffer with read/write permissions before restricting the buffer
> to being mapped as read-only for future mappings.
>
> The resulting VMA from the writable mapping has VM_MAYEXEC set, meaning
> that mprotect() can change the mapping to be executable. Therefore,
> implementing the seal similar to F_SEAL_WRITE would not be appropriate,
> since it would not work with the CursorWindow usecase. This is because
> the CursorWindow process restricts the mapping permissions to read-only
> after the writable mapping is created. So, adding a file seal for
> executable mappings that operates like F_SEAL_WRITE would fail.
>
> Therefore, add support for F_SEAL_FUTURE_EXEC, which is handled
> similarly to F_SEAL_FUTURE_WRITE. This ensures that CursorWindow can
> continue to create a writable mapping initially, and then restrict the
> permissions on the buffer to be mappable as read-only by using both
> F_SEAL_FUTURE_WRITE and F_SEAL_FUTURE_EXEC. After the seal is
> applied, any calls to mmap() with PROT_EXEC will fail.
>
> [1] https://cs.android.com/android/kernel/superproject/+/common-android-mainline:common/drivers/staging/android/ashmem.c
> [2] https://developer.android.com/reference/android/database/CursorWindow
>
> Cc: Suren Baghdasaryan <[e-mail]>
> Cc: Kalesh Singh <[e-mail]>
> Cc: John Stultz <[e-mail]>
> Signed-off-by: Isaac J. Manjarres <[e-mail]>
> ---
>  include/linux/mm.h         |  5 +++++
>  include/uapi/linux/fcntl.h |  1 +
>  mm/memfd.c                 |  1 +
>  mm/mmap.c                  | 11 +++++++++++
>  4 files changed, 18 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 4eb8e62d5c67..40c03a491e45 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4096,6 +4096,11 @@ static inline bool is_write_sealed(int seals)
>         return seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE);
>  }
>
> +static inline bool is_exec_sealed(int seals)
> +{
> +       return seals & F_SEAL_FUTURE_EXEC;
> +}
> +
>  /**
>   * is_readonly_sealed - Checks whether write-sealed but mapped read-only,
>   *                      in which case writes should be disallowing moving
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 6e6907e63bfc..ef066e524777 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -49,6 +49,7 @@
>  #define F_SEAL_WRITE   0x0008  /* prevent writes */
>  #define F_SEAL_FUTURE_WRITE    0x0010  /* prevent future writes while mapped */
>  #define F_SEAL_EXEC    0x0020  /* prevent chmod modifying exec bits */
> +#define F_SEAL_FUTURE_EXEC     0x0040 /* prevent future executable mappings */
>  /* (1U << 31) is reserved for signed error codes */
>
>  /*
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 35a370d75c9a..77b49995a044 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -184,6 +184,7 @@ unsigned int *memfd_file_seals_ptr(struct file *file)
>  }
>
>  #define F_ALL_SEALS (F_SEAL_SEAL | \
> +                    F_SEAL_FUTURE_EXEC |\
>                      F_SEAL_EXEC | \
>                      F_SEAL_SHRINK | \
>                      F_SEAL_GROW | \
> diff --git a/mm/mmap.c b/mm/mmap.c
> index b1b2a24ef82e..c7b96b057fda 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -375,6 +375,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>                 if (!file_mmap_ok(file, inode, pgoff, len))
>                         return -EOVERFLOW;
>
> +               if (is_exec_sealed(seals)) {
> +                       /* No new executable mappings if the file is exec sealed. */
> +                       if (prot & PROT_EXEC)
> +                               return -EACCES;

I think this should be -EPERM to be consistent with seal_check_write()
and mmap(2) man page:

" EPERM The operation was prevented by a file seal; see fcntl(2)."

Thanks,
Kalesh

> +                       /*
> +                        * Prevent an initially non-executable mapping from
> +                        * later becoming executable via mprotect().
> +                        */
> +                       vm_flags &= ~VM_MAYEXEC;
> +               }
> +
>                 flags_mask = LEGACY_MAP_MASK;
>                 if (file->f_op->fop_flags & FOP_MMAP_SYNC)
>                         flags_mask |= MAP_SYNC;
> --
> 2.47.0.338.g60cca15819-goog
>

Lorenzo Stoakes Dec. 6, 2024, 6:19 p.m. UTC | #2

On Thu, Dec 05, 2024 at 05:09:22PM -0800, Isaac J. Manjarres wrote:
> Android currently uses the ashmem driver [1] for creating shared memory
> regions between processes. Ashmem buffers can initially be mapped with
> PROT_READ, PROT_WRITE, and PROT_EXEC. Processes can then use the
> ASHMEM_SET_PROT_MASK ioctl command to restrict--never add--the
> permissions that the buffer can be mapped with.
>
> Processes can remove the ability to map ashmem buffers as executable to
> ensure that those buffers cannot be exploited to run unintended code.
> We are currently trying to replace ashmem with memfd. However, memfd
> does not have a provision to permanently remove the ability to map a
> buffer as executable. Although, this should be something that can be
> achieved via a new file seal.
>
> There are known usecases (e.g. CursorWindow [2]) where a process
> maps a buffer with read/write permissions before restricting the buffer
> to being mapped as read-only for future mappings.
>
> The resulting VMA from the writable mapping has VM_MAYEXEC set, meaning
> that mprotect() can change the mapping to be executable. Therefore,
> implementing the seal similar to F_SEAL_WRITE would not be appropriate,
> since it would not work with the CursorWindow usecase. This is because
> the CursorWindow process restricts the mapping permissions to read-only
> after the writable mapping is created. So, adding a file seal for
> executable mappings that operates like F_SEAL_WRITE would fail.
>
> Therefore, add support for F_SEAL_FUTURE_EXEC, which is handled
> similarly to F_SEAL_FUTURE_WRITE. This ensures that CursorWindow can
> continue to create a writable mapping initially, and then restrict the
> permissions on the buffer to be mappable as read-only by using both
> F_SEAL_FUTURE_WRITE and F_SEAL_FUTURE_EXEC. After the seal is
> applied, any calls to mmap() with PROT_EXEC will fail.
>
> [1] https://cs.android.com/android/kernel/superproject/+/common-android-mainline:common/drivers/staging/android/ashmem.c
> [2] https://developer.android.com/reference/android/database/CursorWindow
>
> Cc: Suren Baghdasaryan <[e-mail]>
> Cc: Kalesh Singh <[e-mail]>
> Cc: John Stultz <[e-mail]>
> Signed-off-by: Isaac J. Manjarres <[e-mail]>
> ---
>  include/linux/mm.h         |  5 +++++
>  include/uapi/linux/fcntl.h |  1 +
>  mm/memfd.c                 |  1 +
>  mm/mmap.c                  | 11 +++++++++++
>  4 files changed, 18 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 4eb8e62d5c67..40c03a491e45 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4096,6 +4096,11 @@ static inline bool is_write_sealed(int seals)
>  	return seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE);
>  }
>
> +static inline bool is_exec_sealed(int seals)
> +{
> +	return seals & F_SEAL_FUTURE_EXEC;
> +}
> +
>  /**
>   * is_readonly_sealed - Checks whether write-sealed but mapped read-only,
>   *                      in which case writes should be disallowing moving
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 6e6907e63bfc..ef066e524777 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -49,6 +49,7 @@
>  #define F_SEAL_WRITE	0x0008	/* prevent writes */
>  #define F_SEAL_FUTURE_WRITE	0x0010  /* prevent future writes while mapped */
>  #define F_SEAL_EXEC	0x0020  /* prevent chmod modifying exec bits */
> +#define F_SEAL_FUTURE_EXEC	0x0040 /* prevent future executable mappings */
>  /* (1U << 31) is reserved for signed error codes */
>
>  /*
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 35a370d75c9a..77b49995a044 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -184,6 +184,7 @@ unsigned int *memfd_file_seals_ptr(struct file *file)
>  }
>
>  #define F_ALL_SEALS (F_SEAL_SEAL | \
> +		     F_SEAL_FUTURE_EXEC |\
>  		     F_SEAL_EXEC | \
>  		     F_SEAL_SHRINK | \
>  		     F_SEAL_GROW | \
> diff --git a/mm/mmap.c b/mm/mmap.c
> index b1b2a24ef82e..c7b96b057fda 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -375,6 +375,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>  		if (!file_mmap_ok(file, inode, pgoff, len))
>  			return -EOVERFLOW;
>

Not maybe in favour of _where_ in the logic we check this and definitely
not in expanding this do_mmap() stuff much further.

See comment at bottom though... I have a cunning plan :)

> +		if (is_exec_sealed(seals)) {

Are we intentionally disallowing a MAP_PRIVATE memfd's mapping's execution?
I've not tested this scenario so don't know if we somehow disallow this in
another way but note on write checks we only care about shared mappings.

I mean one could argue that a MAP_PRIVATE situation is the same as copying
the data into an anon buffer and doing what you want with it, here you
could argue the same...

So probably we should only care about VM_SHARED?

> +			/* No new executable mappings if the file is exec sealed. */
> +			if (prot & PROT_EXEC)

Seems strange to reference a prot flag rather than vma flag, we should have
that set up by now.

> +				return -EACCES;
> +			/*
> +			 * Prevent an initially non-executable mapping from
> +			 * later becoming executable via mprotect().
> +			 */
> +			vm_flags &= ~VM_MAYEXEC;
> +		}
> +

You know, I'm in two minds about this... I explicitly moved logic to
do_mmap() in [0] to workaround a chicken-and-egg scenario with having
accidentally undone the ability to mmap() read-only F_WRITE_SEALed
mappings, which meant I 'may as well' move the 'future proofing' clearing
of VM_MAYWRITE for F_SEAL_FUTURE_WRITE too.

But now I feel that the use of shmem_mmap() and hugetlbfs_file_mmap() to do
_any_ of this is pretty odious in general, we may as well do it all
upfront.

[0]:https://lore.kernel.org/all/[e-mail]/

>  		flags_mask = LEGACY_MAP_MASK;
>  		if (file->f_op->fop_flags & FOP_MMAP_SYNC)
>  			flags_mask |= MAP_SYNC;
> --
> 2.47.0.338.g60cca15819-goog
>

So actually - overall - could you hold off a bit on this until I've had a
think and can perhaps send a patch that refactors this?

Then your patch can build on top of that one and we can handle this all in
one place and sanely :)

Sorry you've kicked off thought processes here and that's often a dangerous
thing :P

Isaac J. Manjarres Dec. 6, 2024, 8:48 p.m. UTC | #3

On Fri, Dec 06, 2024 at 06:19:49PM +0000, Lorenzo Stoakes wrote:
> On Thu, Dec 05, 2024 at 05:09:22PM -0800, Isaac J. Manjarres wrote:
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index b1b2a24ef82e..c7b96b057fda 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -375,6 +375,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> >  		if (!file_mmap_ok(file, inode, pgoff, len))
> >  			return -EOVERFLOW;
> >
> 
> Not maybe in favour of _where_ in the logic we check this and definitely
> not in expanding this do_mmap() stuff much further.
> 
> See comment at bottom though... I have a cunning plan :)
> 
> > +		if (is_exec_sealed(seals)) {
> 
> Are we intentionally disallowing a MAP_PRIVATE memfd's mapping's execution?
> I've not tested this scenario so don't know if we somehow disallow this in
> another way but note on write checks we only care about shared mappings.
> 
> I mean one could argue that a MAP_PRIVATE situation is the same as copying
> the data into an anon buffer and doing what you want with it, here you
> could argue the same...
> 
> So probably we should only care about VM_SHARED?

Thanks for taking a look at this!

I'd originally implemented it for just the VM_SHARED case, but after
discussing it with Kalesh, I changed it to disallow executable
mappings for both MAP_SHARED and MAP_PRIVATE.

Our thought was that write sealing didn't apply in the MAP_PRIVATE case
to support COW with MAP_PRIVATE. There's nothing similar to COW with
execution, so I decided to prevent it for both cases; it also retains
the same behavior as the ashmem driver.

> > +			/* No new executable mappings if the file is exec sealed. */
> > +			if (prot & PROT_EXEC)
> 
> Seems strange to reference a prot flag rather than vma flag, we should have
> that set up by now.

That makes sense. I can change this to check for VM_EXEC in v2 of this
series.
> > +				return -EACCES;
> > +			/*
> > +			 * Prevent an initially non-executable mapping from
> > +			 * later becoming executable via mprotect().
> > +			 */
> > +			vm_flags &= ~VM_MAYEXEC;
> > +		}
> > +
> 
> You know, I'm in two minds about this... I explicitly moved logic to
> do_mmap() in [0] to workaround a chicken-and-egg scenario with having
> accidentally undone the ability to mmap() read-only F_WRITE_SEALed
> mappings, which meant I 'may as well' move the 'future proofing' clearing
> of VM_MAYWRITE for F_SEAL_FUTURE_WRITE too.
> 
> But now I feel that the use of shmem_mmap() and hugetlbfs_file_mmap() to do
> _any_ of this is pretty odious in general, we may as well do it all
> upfront.
> 
> [0]:https://lore.kernel.org/all/[e-mail]/

I agree. I really like the idea of handling the future proofing and
error checking in one place. It makes understanding how these seals
work a lot easier, and has the added benefit of only worrying about
the check once rather than having to duplicate it in both shmem_mmap() and
hugetlbfs_file_mmap().

> >  		flags_mask = LEGACY_MAP_MASK;
> >  		if (file->f_op->fop_flags & FOP_MMAP_SYNC)
> >  			flags_mask |= MAP_SYNC;
> > --
> > 2.47.0.338.g60cca15819-goog
> >
> 
> So actually - overall - could you hold off a bit on this until I've had a
> think and can perhaps send a patch that refactors this?
> 
> Then your patch can build on top of that one and we can handle this all in
> one place and sanely :)
> 
> Sorry you've kicked off thought processes here and that's often a dangerous
> thing :P
Thanks again for reviewing these patches! Happy that I was able to get
the gears turning :)

I'm really interested in helping with this, so is there any forum you'd
like to use for collaborating on this or any way I can help?

I'm also more than happy to test out any patches that you'd like!

Thanks,
Isaac

Isaac J. Manjarres Dec. 6, 2024, 8:50 p.m. UTC | #4

On Fri, Dec 06, 2024 at 09:49:35AM -0800, Kalesh Singh wrote:
> On Thu, Dec 5, 2024 at 5:09 PM Isaac J. Manjarres
> <[e-mail]> wrote:
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -375,6 +375,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> >                 if (!file_mmap_ok(file, inode, pgoff, len))
> >                         return -EOVERFLOW;
> >
> > +               if (is_exec_sealed(seals)) {
> > +                       /* No new executable mappings if the file is exec sealed. */
> > +                       if (prot & PROT_EXEC)
> > +                               return -EACCES;
> 
> I think this should be -EPERM to be consistent with seal_check_write()
> and mmap(2) man page:
> 
> " EPERM The operation was prevented by a file seal; see fcntl(2)."
> 
> Thanks,
> Kalesh
> 

Thanks for catching that Kalesh! I agree and will fix this in v2 of the
series.

Thanks,
Isaac

Lorenzo Stoakes Dec. 6, 2024, 9:14 p.m. UTC | #5

On Fri, Dec 06, 2024 at 12:48:09PM -0800, Isaac Manjarres wrote:
> On Fri, Dec 06, 2024 at 06:19:49PM +0000, Lorenzo Stoakes wrote:
> > On Thu, Dec 05, 2024 at 05:09:22PM -0800, Isaac J. Manjarres wrote:
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index b1b2a24ef82e..c7b96b057fda 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -375,6 +375,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> > >  		if (!file_mmap_ok(file, inode, pgoff, len))
> > >  			return -EOVERFLOW;
> > >
> >
> > Not maybe in favour of _where_ in the logic we check this and definitely
> > not in expanding this do_mmap() stuff much further.
> >
> > See comment at bottom though... I have a cunning plan :)
> >
> > > +		if (is_exec_sealed(seals)) {
> >
> > Are we intentionally disallowing a MAP_PRIVATE memfd's mapping's execution?
> > I've not tested this scenario so don't know if we somehow disallow this in
> > another way but note on write checks we only care about shared mappings.
> >
> > I mean one could argue that a MAP_PRIVATE situation is the same as copying
> > the data into an anon buffer and doing what you want with it, here you
> > could argue the same...
> >
> > So probably we should only care about VM_SHARED?
>
> Thanks for taking a look at this!
>
> I'd originally implemented it for just the VM_SHARED case, but after
> discussing it with Kalesh, I changed it to disallow executable
> mappings for both MAP_SHARED and MAP_PRIVATE.
>
> Our thought was that write sealing didn't apply in the MAP_PRIVATE case
> to support COW with MAP_PRIVATE. There's nothing similar to COW with
> execution, so I decided to prevent it for both cases; it also retains
> the same behavior as the ashmem driver.

Hm, yeah I'm not sure that's really justified, I mean what's to stop a
caller from just mapping their own memory mapping executable, copying the
data and executing?

There's also further concerns around execution restriction for instance in
memfd_add_seals():

	/*
	 * SEAL_EXEC implys SEAL_WRITE, making W^X from the start.
	 */
	if (seals & F_SEAL_EXEC && inode->i_mode & 0111)
		seals |= F_SEAL_SHRINK|F_SEAL_GROW|F_SEAL_WRITE|F_SEAL_FUTURE_WRITE;

So you probably want to change this to include F_SEAL_FUTURE_EXEC, and note
your proposal interacts negatively with the whole
MFD_NOEXEC_SCOPE_NOEXEC_ENFORCED mode set in vm.memfd_noeec - any system
with this set to '2' will literally not allow you to do what you want if
set to 2.

See https://origin.kernel.org/doc/html/latest/userspace-api/mfd_noexec.html

>
> > > +			/* No new executable mappings if the file is exec sealed. */
> > > +			if (prot & PROT_EXEC)
> >
> > Seems strange to reference a prot flag rather than vma flag, we should have
> > that set up by now.
>
> That makes sense. I can change this to check for VM_EXEC in v2 of this
> series.
> > > +				return -EACCES;
> > > +			/*
> > > +			 * Prevent an initially non-executable mapping from
> > > +			 * later becoming executable via mprotect().
> > > +			 */
> > > +			vm_flags &= ~VM_MAYEXEC;
> > > +		}
> > > +
> >
> > You know, I'm in two minds about this... I explicitly moved logic to
> > do_mmap() in [0] to workaround a chicken-and-egg scenario with having
> > accidentally undone the ability to mmap() read-only F_WRITE_SEALed
> > mappings, which meant I 'may as well' move the 'future proofing' clearing
> > of VM_MAYWRITE for F_SEAL_FUTURE_WRITE too.
> >
> > But now I feel that the use of shmem_mmap() and hugetlbfs_file_mmap() to do
> > _any_ of this is pretty odious in general, we may as well do it all
> > upfront.
> >
> > [0]:https://lore.kernel.org/all/[e-mail]/
>
> I agree. I really like the idea of handling the future proofing and
> error checking in one place. It makes understanding how these seals
> work a lot easier, and has the added benefit of only worrying about
> the check once rather than having to duplicate it in both shmem_mmap() and
> hugetlbfs_file_mmap().
>
> > >  		flags_mask = LEGACY_MAP_MASK;
> > >  		if (file->f_op->fop_flags & FOP_MMAP_SYNC)
> > >  			flags_mask |= MAP_SYNC;
> > > --
> > > 2.47.0.338.g60cca15819-goog
> > >
> >
> > So actually - overall - could you hold off a bit on this until I've had a
> > think and can perhaps send a patch that refactors this?
> >
> > Then your patch can build on top of that one and we can handle this all in
> > one place and sanely :)
> >
> > Sorry you've kicked off thought processes here and that's often a dangerous
> > thing :P
> Thanks again for reviewing these patches! Happy that I was able to get
> the gears turning :)
>
> I'm really interested in helping with this, so is there any forum you'd
> like to use for collaborating on this or any way I can help?
>
> I'm also more than happy to test out any patches that you'd like!

Thanks, I'm just going to post to the mailing list, this is the discussion
forum I'm making use of for this :)

I will cc- you on my patch and definitely I'd appreciate testing thanks!

But yeah, to be clear I'm not done with reviewing this, I need more time to
digest what you're trying to do here, but you definitely need to think
about the exec limitations.

>
> Thanks,
> Isaac

Isaac J. Manjarres Dec. 11, 2024, 8:56 p.m. UTC | #6

On Fri, Dec 06, 2024 at 09:14:58PM +0000, Lorenzo Stoakes wrote:
> On Fri, Dec 06, 2024 at 12:48:09PM -0800, Isaac Manjarres wrote:
> > On Fri, Dec 06, 2024 at 06:19:49PM +0000, Lorenzo Stoakes wrote:
> > > On Thu, Dec 05, 2024 at 05:09:22PM -0800, Isaac J. Manjarres wrote:
> > > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > > index b1b2a24ef82e..c7b96b057fda 100644
> > > > --- a/mm/mmap.c
> > > > +++ b/mm/mmap.c
> > > > @@ -375,6 +375,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> > > >  		if (!file_mmap_ok(file, inode, pgoff, len))
> > > >  			return -EOVERFLOW;
> > > >
> > >
> > > Not maybe in favour of _where_ in the logic we check this and definitely
> > > not in expanding this do_mmap() stuff much further.
> > >
> > > See comment at bottom though... I have a cunning plan :)
> > >
> > > > +		if (is_exec_sealed(seals)) {
> > >
> > > Are we intentionally disallowing a MAP_PRIVATE memfd's mapping's execution?
> > > I've not tested this scenario so don't know if we somehow disallow this in
> > > another way but note on write checks we only care about shared mappings.
> > >
> > > I mean one could argue that a MAP_PRIVATE situation is the same as copying
> > > the data into an anon buffer and doing what you want with it, here you
> > > could argue the same...
> > >
> > > So probably we should only care about VM_SHARED?
> >
> > Thanks for taking a look at this!
> >
> > I'd originally implemented it for just the VM_SHARED case, but after
> > discussing it with Kalesh, I changed it to disallow executable
> > mappings for both MAP_SHARED and MAP_PRIVATE.
> >
> > Our thought was that write sealing didn't apply in the MAP_PRIVATE case
> > to support COW with MAP_PRIVATE. There's nothing similar to COW with
> > execution, so I decided to prevent it for both cases; it also retains
> > the same behavior as the ashmem driver.
> 
> Hm, yeah I'm not sure that's really justified, I mean what's to stop a
> caller from just mapping their own memory mapping executable, copying the
> data and executing?
> 
That's a fair point. In that case, I think it makes sense to enforce the
seal only when the mapping is shared.

The case I'm trying to address is when a process (A) allocates a memfd
that is meant to be read and written by itself and another process (B).
A shares the buffer with B, but B injects code into the buffer, and
compromises A such that A maps the buffer with PROT_EXEC and runs the
code that B injected into it.

If A used F_SEAL_FUTURE_EXEC prior to sharing the buffer, then it could
reduce the attack surface on itself in this scenario.

> There's also further concerns around execution restriction for instance in
> memfd_add_seals():
> 
> 	/*
> 	 * SEAL_EXEC implys SEAL_WRITE, making W^X from the start.
> 	 */
> 	if (seals & F_SEAL_EXEC && inode->i_mode & 0111)
> 		seals |= F_SEAL_SHRINK|F_SEAL_GROW|F_SEAL_WRITE|F_SEAL_FUTURE_WRITE;
> 
> So you probably want to change this to include F_SEAL_FUTURE_EXEC, and note
Do you mean adding a case where if F_SEAL_FUTURE_EXEC is in the seals,
then we should clear the X bits of the file and use F_SEAL_EXEC as well?

I don't think the case in the if condition should imply F_SEAL_FUTURE_EXEC,
since the file is still executable in this case?

> your proposal interacts negatively with the whole
> MFD_NOEXEC_SCOPE_NOEXEC_ENFORCED mode set in vm.memfd_noeec - any system
> with this set to '2' will literally not allow you to do what you want if
> set to 2.
> 
> See https://origin.kernel.org/doc/html/latest/userspace-api/mfd_noexec.html
Sorry, I didn't follow how this would impact
MFD_NOEXEC_SCOPE_NOEXEC_ENFORCED. Could you please clarify that?

> > Thanks again for reviewing these patches! Happy that I was able to get
> > the gears turning :)
> >
> > I'm really interested in helping with this, so is there any forum you'd
> > like to use for collaborating on this or any way I can help?
> >
> > I'm also more than happy to test out any patches that you'd like!
> 
> Thanks, I'm just going to post to the mailing list, this is the discussion
> forum I'm making use of for this :)
> 
> I will cc- you on my patch and definitely I'd appreciate testing thanks!
> 
> But yeah, to be clear I'm not done with reviewing this, I need more time to
> digest what you're trying to do here, but you definitely need to think
> about the exec limitations.

Thanks for sending out the patch. I took a look and tested it out and it
definitely makes implementing this a lot nicer!

Thanks,
Isaac

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 4eb8e62d5c67..40c03a491e45 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4096,6 +4096,11 @@  static inline bool is_write_sealed(int seals)
 	return seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE);
 }
 
+static inline bool is_exec_sealed(int seals)
+{
+	return seals & F_SEAL_FUTURE_EXEC;
+}
+
 /**
  * is_readonly_sealed - Checks whether write-sealed but mapped read-only,
  *                      in which case writes should be disallowing moving
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 6e6907e63bfc..ef066e524777 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -49,6 +49,7 @@ 
 #define F_SEAL_WRITE	0x0008	/* prevent writes */
 #define F_SEAL_FUTURE_WRITE	0x0010  /* prevent future writes while mapped */
 #define F_SEAL_EXEC	0x0020  /* prevent chmod modifying exec bits */
+#define F_SEAL_FUTURE_EXEC	0x0040 /* prevent future executable mappings */
 /* (1U << 31) is reserved for signed error codes */
 
 /*
diff --git a/mm/memfd.c b/mm/memfd.c
index 35a370d75c9a..77b49995a044 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -184,6 +184,7 @@  unsigned int *memfd_file_seals_ptr(struct file *file)
 }
 
 #define F_ALL_SEALS (F_SEAL_SEAL | \
+		     F_SEAL_FUTURE_EXEC |\
 		     F_SEAL_EXEC | \
 		     F_SEAL_SHRINK | \
 		     F_SEAL_GROW | \
diff --git a/mm/mmap.c b/mm/mmap.c
index b1b2a24ef82e..c7b96b057fda 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -375,6 +375,17 @@  unsigned long do_mmap(struct file *file, unsigned long addr,
 		if (!file_mmap_ok(file, inode, pgoff, len))
 			return -EOVERFLOW;
 
+		if (is_exec_sealed(seals)) {
+			/* No new executable mappings if the file is exec sealed. */
+			if (prot & PROT_EXEC)
+				return -EACCES;
+			/*
+			 * Prevent an initially non-executable mapping from
+			 * later becoming executable via mprotect().
+			 */
+			vm_flags &= ~VM_MAYEXEC;
+		}
+
 		flags_mask = LEGACY_MAP_MASK;
 		if (file->f_op->fop_flags & FOP_MMAP_SYNC)
 			flags_mask |= MAP_SYNC;