On Thu, Oct 17, 2024 at 12:51:04AM +0000, jeffxu@xxxxxxxxxxxx wrote: > From: Jeff Xu <jeffxu@xxxxxxxxxxxx> > > Two fixes for madvise(MADV_DONTNEED) when sealed. > > For PROT_NONE mappings, the previous blocking of > madvise(MADV_DONTNEED) is unnecessary. As PROT_NONE already prohibits > memory access, madvise(MADV_DONTNEED) should be allowed to proceed in > order to free the page. > > For file-backed, private, read-only memory mappings, we previously did > not block the madvise(MADV_DONTNEED). This was based on > the assumption that the memory's content, being file-backed, could be > retrieved from the file if accessed again. However, this assumption > failed to consider scenarios where a mapping is initially created as > read-write, modified, and subsequently changed to read-only. The newly > introduced VM_WASWRITE flag addresses this oversight. > > Reported-by: Pedro Falcato <pedro.falcato@xxxxxxxxx> > Link:https://lore.kernel.org/all/CABi2SkW2XzuZ2-TunWOVzTEX1qc29LhjfNQ3hD4Nym8U-_f+ug@xxxxxxxxxxxxxx/ > Fixes: 8be7258aad44 ("mseal: add mseal syscall") > Cc: <stable@xxxxxxxxxxxxxxx> # 6.11.y: 4d1b3416659b: mm: move can_modify_vma to mm/vma.h > Cc: <stable@xxxxxxxxxxxxxxx> # 6.11.y: 4a2dd02b0916: mm/mprotect: replace can_modify_mm with can_modify_vma > Cc: <stable@xxxxxxxxxxxxxxx> # 6.11.y: 23c57d1fa2b9: mseal: replace can_modify_mm_madv with a vma variant > Cc: <stable@xxxxxxxxxxxxxxx> # 6.11.y > Signed-off-by: Jeff Xu <jeffxu@xxxxxxxxxxxx> > --- > include/linux/mm.h | 2 ++ > mm/mprotect.c | 3 +++ > mm/mseal.c | 42 ++++++++++++++++++++++++++++++++++++------ > 3 files changed, 41 insertions(+), 6 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 4c32003c8404..b402eca2565a 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -430,6 +430,8 @@ extern unsigned int kobjsize(const void *objp); > #ifdef CONFIG_64BIT > /* VM is sealed, in vm_flags */ > #define VM_SEALED _BITUL(63) > +/* VM was writable */ Woefully poor and misleading comment. > +#define VM_WASWRITE _BITUL(62) The bar for an additional VMA flag is _really high_. As far as I'm concerned you absolutely do not hit that bar here. > #endif > > /* Bits set in the VMA until the stack is in its final location */ > diff --git a/mm/mprotect.c b/mm/mprotect.c > index 0c5d6d06107d..6397135ca526 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -821,6 +821,9 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > break; > } > > + if ((vma->vm_flags & VM_WRITE) && !(newflags & VM_WRITE)) > + newflags |= VM_WASWRITE; > + You're making this unmergeable now!!! No! Lord this is horrid. You can't fundamentally change how mprotect() functions to suit edge cases for mseal, sorry. > error = security_file_mprotect(vma, reqprot, prot); > if (error) > break; > diff --git a/mm/mseal.c b/mm/mseal.c > index ece977bd21e1..28f28487be17 100644 > --- a/mm/mseal.c > +++ b/mm/mseal.c > @@ -36,12 +36,8 @@ static bool is_madv_discard(int behavior) > return false; > } > > -static bool is_ro_anon(struct vm_area_struct *vma) > +static bool anon_is_ro(struct vm_area_struct *vma) > { > - /* check anonymous mapping. */ > - if (vma->vm_file || vma->vm_flags & VM_SHARED) > - return false; > - > /* > * check for non-writable: > * PROT=RO or PKRU is not writeable. > @@ -53,6 +49,22 @@ static bool is_ro_anon(struct vm_area_struct *vma) > return false; > } > > +static bool vma_is_prot_none(struct vm_area_struct *vma) > +{ > + if ((vma->vm_flags & VM_ACCESS_FLAGS) == VM_NONE) > + return true; > + > + return false; > +} You don't need this, there is already vma_is_accessible() in mm.h. > + > +static bool vma_was_writable_turn_readonly(struct vm_area_struct *vma) > +{ > + if (!(vma->vm_flags & VM_WRITE) && vma->vm_flags & VM_WASWRITE) > + return true; > + > + return false; > +} The naming of this is horrid and confusing. > + > /* > * Check if a vma is allowed to be modified by madvise. > */ > @@ -61,7 +73,25 @@ bool can_modify_vma_madv(struct vm_area_struct *vma, int behavior) > if (!is_madv_discard(behavior)) > return true; > > - if (unlikely(!can_modify_vma(vma) && is_ro_anon(vma))) > + /* not sealed */ > + if (likely(can_modify_vma(vma))) Please don't just use likely() / unlikely() because _you_ think they're likely/unlikely. Only use them based on profiling data. if you don't have it, remove them. > + return true; > + > + /* PROT_NONE mapping */ Useless comment. > + if (vma_is_prot_none(vma)) > + return true; > + > + /* file-backed private mapping */ Err... how do you know it's a private mapping? > + if (vma->vm_file) { > + /* read-only but was writeable */ > + if (vma_was_writable_turn_readonly(vma)) > + return false; This whole thing seems broken, and we already have a mechanism for this, see mapping_writably_mapped() which _also_ handles write seals for memfd's which you are not accounting for here. > + > + return true; > + } > + > + /* anonymous mapping is read-only */ > + if (anon_is_ro(vma)) You're implementing subtle details here with 1 line comments (that are pretty well useless), that's just not good enough. Please make sure to add _meaningful_ comments that will help another developer understand what's going on. > return false; > > /* Allow by default. */ > -- > 2.47.0.rc1.288.g06298d1525-goog >
