permit write-sealed memfd read-only shared mappings

In order for a F_SEAL_WRITE sealed memfd mapping to have an opportunity to clear VM_MAYWRITE, we must be able to invoke the appropriate vm_ops->mmap() handler to do so. We would otherwise fail the mapping_map_writable() check before we had the opportunity to avoid it.

This patch moves this check after the call_mmap() invocation. Only memfd actively denies write access causing a potential failure here (in memfd_add_seals()), so there should be no impact on non-memfd cases.

This patch makes the userland-visible change that MAP_SHARED, PROT_READ mappings of an F_SEAL_WRITE sealed memfd mapping will now succeed.

Signed-off-by Lorenzo Stoakes <[email protected]>

---
 mm/mmap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

mm/mmap.c +0 -5

index c96dcce90772..a166e9f3c474 100644
@@ -2596,17 +2596,17 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	vma->vm_pgoff = pgoff;
 
 	if (file) {
-		if (is_shared_maywrite(vm_flags)) {
-			error = mapping_map_writable(file->f_mapping);
-			if (error)
-				goto free_vma;
-		}

Quoted 36 lines · Lorenzo Stoakes

In order for a F_SEAL_WRITE sealed memfd mapping to have an opportunity to clear VM_MAYWRITE, we must be able to invoke the appropriate vm_ops->mmap() handler to do so. We would otherwise fail the mapping_map_writable() check before we had the opportunity to avoid it.

This patch moves this check after the call_mmap() invocation. Only memfd actively denies write access causing a potential failure here (in memfd_add_seals()), so there should be no impact on non-memfd cases.

This patch makes the userland-visible change that MAP_SHARED, PROT_READ mappings of an F_SEAL_WRITE sealed memfd mapping will now succeed.

Signed-off-by Lorenzo Stoakes <[email protected]>

---
 mm/mmap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

mm/mmap.c +0 -6

index c96dcce90772..a166e9f3c474 100644
@@ -2596,17 +2596,17 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	vma->vm_pgoff = pgoff;
 
 	if (file) {
-		if (is_shared_maywrite(vm_flags)) {
-			error = mapping_map_writable(file->f_mapping);
-			if (error)
-				goto free_vma;
-		}
-
 		vma->vm_file = get_file(file);
 		error = call_mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;
 

mm/mmap.c unsigned long mmap_region(struct file *file, unsigned long addr, · line 2607

Quoted 4 lines

mm/mmap.c +4 -0

+		if (vma_is_shared_maywrite(vma)) {
+			error = mapping_map_writable(file->f_mapping);
+			if (error)
+				goto unmap_and_free_vma;

Shouldn't we rather jump to close_and_free_vma?

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

This patch series is in two parts:-

1. Currently there are a number of places in the kernel where we assume VM_SHARED implies that a mapping is writable. Let's be slightly less strict and relax this restriction in the case that VM_MAYWRITE is not set.

This should have no noticeable impact as the lack of VM_MAYWRITE implies that the mapping can not be made writable via mprotect() or any other means.

2. Align the behaviour of F_SEAL_WRITE and F_SEAL_FUTURE_WRITE on mmap(). The latter already clears the VM_MAYWRITE flag for a sealed read-only mapping, we simply extend this to F_SEAL_WRITE too.

For this to have effect, we must also invoke call_mmap() before mapping_map_writable().

As this is quite a fundamental change on the assumptions around VM_SHARED and since this causes a visible change to userland (in permitting read-only shared mappings on F_SEAL_WRITE mappings), I am putting forward as an RFC to see if there is anything terribly wrong with it.

Hi!

Quoted 18 lines · Lorenzo Stoakes

This patch series is in two parts:-

1. Currently there are a number of places in the kernel where we assume VM_SHARED implies that a mapping is writable. Let's be slightly less strict and relax this restriction in the case that VM_MAYWRITE is not set.

This should have no noticeable impact as the lack of VM_MAYWRITE implies that the mapping can not be made writable via mprotect() or any other means.

2. Align the behaviour of F_SEAL_WRITE and F_SEAL_FUTURE_WRITE on mmap(). The latter already clears the VM_MAYWRITE flag for a sealed read-only mapping, we simply extend this to F_SEAL_WRITE too.

For this to have effect, we must also invoke call_mmap() before mapping_map_writable().

Quoted 4 lines

As this is quite a fundamental change on the assumptions around VM_SHARED and since this causes a visible change to userland (in permitting read-only shared mappings on F_SEAL_WRITE mappings), I am putting forward as an RFC to see if there is anything terribly wrong with it.

So what I miss in this series is what the motivation is. Is it that you need to map F_SEAL_WRITE read-only? Why?

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 21 lines · Jan Kara

Hi!

Quoted 18 lines · Lorenzo Stoakes

This patch series is in two parts:-

1. Currently there are a number of places in the kernel where we assume VM_SHARED implies that a mapping is writable. Let's be slightly less strict and relax this restriction in the case that VM_MAYWRITE is not set.

This should have no noticeable impact as the lack of VM_MAYWRITE implies that the mapping can not be made writable via mprotect() or any other means.

2. Align the behaviour of F_SEAL_WRITE and F_SEAL_FUTURE_WRITE on mmap(). The latter already clears the VM_MAYWRITE flag for a sealed read-only mapping, we simply extend this to F_SEAL_WRITE too.

For this to have effect, we must also invoke call_mmap() before mapping_map_writable().

Quoted 8 lines

Quoted 4 lines

As this is quite a fundamental change on the assumptions around VM_SHARED and since this causes a visible change to userland (in permitting read-only shared mappings on F_SEAL_WRITE mappings), I am putting forward as an RFC to see if there is anything terribly wrong with it.

So what I miss in this series is what the motivation is. Is it that you need to map F_SEAL_WRITE read-only? Why?

This originated from the discussion in [1], which refers to the bug reported in [2]. Essentially the user is write-sealing a memfd then trying to mmap it read-only, but receives an -EPERM error.

F_SEAL_FUTURE_WRITE _does_ explicitly permit this but F_SEAL_WRITE does not.

The fcntl() man page states:

Furthermore, trying to create new shared, writable memory-mappings via mmap(2) will also fail with EPERM.

So the kernel does not behave as the documentation states.

I took the user-supplied repro and slightly modified it, enclosed below. After this patch series, this code works correctly.

I think there's definitely a case for the VM_MAYWRITE part of this patch series even if the memfd bits are not considered useful, as we do seem to make the implicit assumption that MAP_SHARED == writable even if !VM_MAYWRITE which seems odd.

Quoted 44 lines · Lorenzo Stoakes

Quoted 21 lines · Jan Kara

Hi!

Quoted 18 lines · Lorenzo Stoakes

This patch series is in two parts:-

1. Currently there are a number of places in the kernel where we assume VM_SHARED implies that a mapping is writable. Let's be slightly less strict and relax this restriction in the case that VM_MAYWRITE is not set.

This should have no noticeable impact as the lack of VM_MAYWRITE implies that the mapping can not be made writable via mprotect() or any other means.

2. Align the behaviour of F_SEAL_WRITE and F_SEAL_FUTURE_WRITE on mmap(). The latter already clears the VM_MAYWRITE flag for a sealed read-only mapping, we simply extend this to F_SEAL_WRITE too.

For this to have effect, we must also invoke call_mmap() before mapping_map_writable().

Quoted 8 lines

Quoted 4 lines

As this is quite a fundamental change on the assumptions around VM_SHARED and since this causes a visible change to userland (in permitting read-only shared mappings on F_SEAL_WRITE mappings), I am putting forward as an RFC to see if there is anything terribly wrong with it.

So what I miss in this series is what the motivation is. Is it that you need to map F_SEAL_WRITE read-only? Why?

This originated from the discussion in [1], which refers to the bug reported in [2]. Essentially the user is write-sealing a memfd then trying to mmap it read-only, but receives an -EPERM error.

F_SEAL_FUTURE_WRITE _does_ explicitly permit this but F_SEAL_WRITE does not.

The fcntl() man page states:

Furthermore, trying to create new shared, writable memory-mappings via mmap(2) will also fail with EPERM.

So the kernel does not behave as the documentation states.

Quoted 7 lines

I took the user-supplied repro and slightly modified it, enclosed below. After this patch series, this code works correctly.

I think there's definitely a case for the VM_MAYWRITE part of this patch series even if the memfd bits are not considered useful, as we do seem to make the implicit assumption that MAP_SHARED == writable even if !VM_MAYWRITE which seems odd.

Thanks for the explanation! Could you please include this information in the cover letter (perhaps in a form of a short note and reference to the mailing list) for future reference? Thanks!

Honza

--
Jan Kara <[email protected]>
SUSE Labs, CR