[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-api
Subject:    Re: [Bug 217238] New: Creating shared read-only map is denied after add write seal to a memfd
From:       Lorenzo Stoakes <lstoakes () gmail ! com>
Date:       2023-03-30 21:46:17
Message-ID: ZCYDKeuAttQJHm8S () murray
[Download RAW message or body]

On Thu, Mar 30, 2023 at 01:47:48PM -0700, Andy Lutomirski wrote:
> 
> 
> > On Mar 30, 2023, at 12:25 PM, Lorenzo Stoakes <lstoakes@gmail.com> wrote:
> > 
> > On Sat, Mar 25, 2023 at 02:51:05PM +0000, Lorenzo Stoakes wrote:
> > > > On Fri, Mar 24, 2023 at 01:36:46PM -0700, Andrew Morton wrote:
> > > > (switched to email.  Please respond via emailed reply-to-all, not via the
> > > > bugzilla web interface).
> > > > 
> > > > > On Fri, 24 Mar 2023 03:34:23 +0000 bugzilla-daemon@kernel.org wrote:
> > > > 
> > > > > https://bugzilla.kernel.org/show_bug.cgi?id=217238
> > > > > 
> > > > > Bug ID: 217238
> > > > > Summary: Creating shared read-only map is denied after add
> > > > > write seal to a memfd
> > > > > Product: Memory Management
> > > > > Version: 2.5
> > > > > Kernel Version: 6.2.8
> > > > > Hardware: All
> > > > > OS: Linux
> > > > > Tree: Mainline
> > > > > Status: NEW
> > > > > Severity: normal
> > > > > Priority: P1
> > > > > Component: Other
> > > > > Assignee: akpm@linux-foundation.org
> > > > > Reporter: yshuiv7@gmail.com
> > > > > Regression: No
> > > > > 
> > > > > Test case:
> > > > > 
> > > > > int main() {
> > > > > int fd = memfd_create("test", MFD_ALLOW_SEALING);
> > > > > write(fd, "test", 4);
> > > > > fcntl(fd, F_ADD_SEALS, F_SEAL_WRITE);
> > > > > 
> > > > > void *ret = mmap(NULL, 4, PROT_READ, MAP_SHARED, fd, 0);
> > > > > }
> > > > > 
> > > > > This fails with EPERM. This is in contradiction with what's described in \
> > > > > the documentation of F_SEAL_WRITE.
> > > > > 
> > > > > --
> > > > > You may reply to this email to add a comment.
> > > > > 
> > > > > You are receiving this mail because:
> > > > > You are the assignee for the bug.
> > > > 
> > > 
> > > This issue seems to be the result of the use of the memfd's shmem region's
> > > page cache object (struct address_space)'s i_mmap_writable field to denote
> > > whether it is write-sealed.
> > > 
> > > The kernel assumes that a VM_SHARED mapping might become writable at any
> > > time via mprotect(), therefore treats VM_SHARED mappings as if they were
> > > writable as far as i_mmap_writable is concerned (this field's primary use
> > > is to determine whether, for architectures that require it, flushing must
> > > occur if this is set to avoid aliasing, see filemap_read() for example).
> > > 
> > > In theory we could convert all such checks to VM_SHARED | VM_WRITE
> > > (importantly including on fork) and then update mprotect() to check
> > > mapping_map_writable() if a user tries to make unwritable memory
> > > writable.
> > > 
> 
> Unless I'm missing something, we have VM_MAYWRITE for almost exactly this purpose.  \
> Can't we just make a shared mapping with both of these bits clear? 

That's a good point, and there's definitely quite a few places where
VM_SHARED is simply taken to imply writable which is a little irksome,
however sprinkling some VM_MAYWRITE checks in these places would resolve
that.

Let me take a look into this and perhaps spin up a RFC to iron out the
 details if this is indeed viable.

> > > I suspect however there are reasons relating to locking that make it
> > > unreasonable to try to do this, but I may be mistaken (others might have
> > > some insight on this). I also see some complexity around this in the
> > > security checks on marking shared writable mappings executable (e.g. in
> > > mmap_violation_check()).
> > > 
> > > In any case, it doesn't really make much sense to have a write-sealed
> > > shared mapping, since you're essentially saying 'nothing _at all_ can write
> > > to this' so it may as well be private. The semantics are unfortunate here,
> > > the memory will still be shared read-only by MAP_PRIVATE mappings.
> > > 
> > > A better choice here might be F_SEAL_FUTURE_WRITE (available from kernel
> > > > =5.1) which does permit shared read-only mappings as this is explicitly
> > > checked for in seal_check_future_write() invoked from shmem_mmap().
> > > 
> > > Regardless, I think the conclusion is that this is not a bug, but rather
> > > that the documentation needs to be updated.
> > > 
> > 
> > Adding docs people to cc list (sorry didn't think to do this in first
> > reply).


[prev in list] [next in list] [prev in thread] [next in thread]