| Message ID | 20200707030204.126021-11-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Tue, Jul 07, 2020 at 06:01:50AM +0300, Jarkko Sakkinen wrote: > +++ b/mm/mprotect.c > @@ -603,13 +603,20 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > goto out; > } > > + tmp = vma->vm_end; > + if (tmp > end) > + tmp = end; > + > error = security_file_mprotect(vma, reqprot, prot); > if (error) > goto out; > > - tmp = vma->vm_end; > - if (tmp > end) > - tmp = end; You don't need to move this any more, right? > + if (vma->vm_ops && vma->vm_ops->mprotect) { > + error = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > + if (error) > + goto out; > + } > + > error = mprotect_fixup(vma, &prev, nstart, tmp, newflags); > if (error) > goto out; > -- > 2.25.1 >
On Tue, Jul 07, 2020 at 04:14:24AM +0100, Matthew Wilcox wrote: > On Tue, Jul 07, 2020 at 06:01:50AM +0300, Jarkko Sakkinen wrote: > > +++ b/mm/mprotect.c > > @@ -603,13 +603,20 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > > goto out; > > } > > > > + tmp = vma->vm_end; > > + if (tmp > end) > > + tmp = end; > > + > > error = security_file_mprotect(vma, reqprot, prot); > > if (error) > > goto out; > > > > - tmp = vma->vm_end; > > - if (tmp > end) > > - tmp = end; > > You don't need to move this any more, right? Ya, I was typing up a longer version, you beat me to the punch... The calculation of 'tmp' doesn't need to be moved. The previous incarnation only moved it so that that 'tmp would be available for .may_mprotect(). The only reason .may_mprotect() was placed before security_file_mprotect() was to avoid triggering LSM errors that would have been blocked by .may_mprotect(), but that justification is no longer relevant as the proposed SGX LSM hooks obviously didn't worm their way into this series. > > + if (vma->vm_ops && vma->vm_ops->mprotect) { > > + error = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > > + if (error) > > + goto out; > > + } Based on "... and then the vma owner can do whatever it needs to before calling mprotect_fixup(), which is already not static", my interpretation is that Matthew's intent was to do: if (vma->vm_ops && vma->vm_ops->mprotect) error = = vma->vm_ops->mprotect(vma, nstart, tmp, prot); else error = mprotect_fixup(vma, &prev, nstart, tmp, newflags); if (error) goto out; i.e. make .mprotect() a full replacement as opposed to a prereq hook. > > + > > error = mprotect_fixup(vma, &prev, nstart, tmp, newflags); > > if (error) > > goto out; > > -- > > 2.25.1 > >
On Mon, Jul 06, 2020 at 08:22:54PM -0700, Sean Christopherson wrote: > On Tue, Jul 07, 2020 at 04:14:24AM +0100, Matthew Wilcox wrote: > > > + if (vma->vm_ops && vma->vm_ops->mprotect) { > > > + error = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > > > + if (error) > > > + goto out; > > > + } > > Based on "... and then the vma owner can do whatever it needs to before > calling mprotect_fixup(), which is already not static", my interpretation > is that Matthew's intent was to do: > > if (vma->vm_ops && vma->vm_ops->mprotect) > error = = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > else > error = mprotect_fixup(vma, &prev, nstart, tmp, newflags); > if (error) > goto out; > > i.e. make .mprotect() a full replacement as opposed to a prereq hook. Yes, it was. I was just looking at the next patch to be sure this was how I'd been misunderstood.
On Tue, Jul 07, 2020 at 04:24:08AM +0100, Matthew Wilcox wrote: > On Mon, Jul 06, 2020 at 08:22:54PM -0700, Sean Christopherson wrote: > > On Tue, Jul 07, 2020 at 04:14:24AM +0100, Matthew Wilcox wrote: > > > > + if (vma->vm_ops && vma->vm_ops->mprotect) { > > > > + error = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > > > > + if (error) > > > > + goto out; > > > > + } > > > > Based on "... and then the vma owner can do whatever it needs to before > > calling mprotect_fixup(), which is already not static", my interpretation > > is that Matthew's intent was to do: > > > > if (vma->vm_ops && vma->vm_ops->mprotect) > > error = = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > > else > > error = mprotect_fixup(vma, &prev, nstart, tmp, newflags); > > if (error) > > goto out; > > > > i.e. make .mprotect() a full replacement as opposed to a prereq hook. > > Yes, it was. I was just looking at the next patch to be sure this was > how I'd been misunderstood. I'm don't get this part. If mprotect_fixup is called in the tail of the callback, why it has to be called inside the callback and not be called after the callback? The reason I only part did what you requested was to do only the part of the change that I get. Not to oppose it. /Jarkko
On Tue, Jul 07, 2020 at 04:14:24AM +0100, Matthew Wilcox wrote: > On Tue, Jul 07, 2020 at 06:01:50AM +0300, Jarkko Sakkinen wrote: > > +++ b/mm/mprotect.c > > @@ -603,13 +603,20 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > > goto out; > > } > > > > + tmp = vma->vm_end; > > + if (tmp > end) > > + tmp = end; > > + > > error = security_file_mprotect(vma, reqprot, prot); > > if (error) > > goto out; > > > > - tmp = vma->vm_end; > > - if (tmp > end) > > - tmp = end; > > You don't need to move this any more, right? My bad. /Jarkko
On Tue, Jul 07, 2020 at 07:01:51AM +0300, Jarkko Sakkinen wrote: > On Tue, Jul 07, 2020 at 04:24:08AM +0100, Matthew Wilcox wrote: > > On Mon, Jul 06, 2020 at 08:22:54PM -0700, Sean Christopherson wrote: > > > On Tue, Jul 07, 2020 at 04:14:24AM +0100, Matthew Wilcox wrote: > > > > > + if (vma->vm_ops && vma->vm_ops->mprotect) { > > > > > + error = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > > > > > + if (error) > > > > > + goto out; > > > > > + } > > > > > > Based on "... and then the vma owner can do whatever it needs to before > > > calling mprotect_fixup(), which is already not static", my interpretation > > > is that Matthew's intent was to do: > > > > > > if (vma->vm_ops && vma->vm_ops->mprotect) > > > error = = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > > > else > > > error = mprotect_fixup(vma, &prev, nstart, tmp, newflags); > > > if (error) > > > goto out; > > > > > > i.e. make .mprotect() a full replacement as opposed to a prereq hook. > > > > Yes, it was. I was just looking at the next patch to be sure this was > > how I'd been misunderstood. > > I'm don't get this part. If mprotect_fixup is called in the tail of the > callback, why it has to be called inside the callback and not be called > after the callback? Because that's how every other VM operation works. Look at your implementation of get_unmapped_area() for example.
On Tue, Jul 07, 2020 at 05:10:46AM +0100, Matthew Wilcox wrote: > On Tue, Jul 07, 2020 at 07:01:51AM +0300, Jarkko Sakkinen wrote: > > On Tue, Jul 07, 2020 at 04:24:08AM +0100, Matthew Wilcox wrote: > > > On Mon, Jul 06, 2020 at 08:22:54PM -0700, Sean Christopherson wrote: > > > > On Tue, Jul 07, 2020 at 04:14:24AM +0100, Matthew Wilcox wrote: > > > > > > + if (vma->vm_ops && vma->vm_ops->mprotect) { > > > > > > + error = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > > > > > > + if (error) > > > > > > + goto out; > > > > > > + } > > > > > > > > Based on "... and then the vma owner can do whatever it needs to before > > > > calling mprotect_fixup(), which is already not static", my interpretation > > > > is that Matthew's intent was to do: > > > > > > > > if (vma->vm_ops && vma->vm_ops->mprotect) > > > > error = = vma->vm_ops->mprotect(vma, nstart, tmp, prot); > > > > else > > > > error = mprotect_fixup(vma, &prev, nstart, tmp, newflags); > > > > if (error) > > > > goto out; > > > > > > > > i.e. make .mprotect() a full replacement as opposed to a prereq hook. > > > > > > Yes, it was. I was just looking at the next patch to be sure this was > > > how I'd been misunderstood. > > > > I'm don't get this part. If mprotect_fixup is called in the tail of the > > callback, why it has to be called inside the callback and not be called > > after the callback? > > Because that's how every other VM operation works. Look at your > implementation of get_unmapped_area() for example. I get the point but I don't think that your proposal could work given that mprotect-callback takes neither 'prev' nor 'newflags' as its parameters. The current callback has no means to call mprotect_fixup() properly. It would have to be extended int (*mprotect)(struct vm_area_struct *vma, struct vm_area_struct **pprev, unsigned long start, unsigned long end, unsigned long prot, unsigned long newflags); Is this what you want? /Jarkko
On Wed, Jul 08, 2020 at 05:33:20PM +0300, Jarkko Sakkinen wrote: > I get the point but I don't think that your proposal could work given > that mprotect-callback takes neither 'prev' nor 'newflags' as its > parameters. The current callback has no means to call mprotect_fixup() > properly. > > It would have to be extended > > int (*mprotect)(struct vm_area_struct *vma, > struct vm_area_struct **pprev, unsigned long start, > unsigned long end, unsigned long prot, > unsigned long newflags); > > Is this what you want? https://lore.kernel.org/linux-mm/20200625173050.GF7703@casper.infradead.org/
On Wed, Jul 08, 2020 at 03:37:08PM +0100, Matthew Wilcox wrote: > On Wed, Jul 08, 2020 at 05:33:20PM +0300, Jarkko Sakkinen wrote: > > I get the point but I don't think that your proposal could work given > > that mprotect-callback takes neither 'prev' nor 'newflags' as its > > parameters. The current callback has no means to call mprotect_fixup() > > properly. > > > > It would have to be extended > > > > int (*mprotect)(struct vm_area_struct *vma, > > struct vm_area_struct **pprev, unsigned long start, > > unsigned long end, unsigned long prot, > > unsigned long newflags); > > > > Is this what you want? > > https://lore.kernel.org/linux-mm/20200625173050.GF7703@casper.infradead.org/ Ugh, it's there as it should be. I'm sorry - I just misread the code. I think that should work, and we do not have to do extra conversion inside the callback. There is still one thing that I'm wondering. 'page->vm_max_prot_bits' contains some an union of subset of {VM_READ, VM_WRITE, VM_EXEC}, and 'newflags' can contain other bits set too. The old implementation of sgx_vma_mprotect() is like this: static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start, unsigned long end, unsigned long prot) { return sgx_encl_may_map(vma->vm_private_data, start, end, calc_vm_prot_bits(prot, 0)); } The new one should be probably the implemented along the lines of static int sgx_vma_mprotect(struct vm_area_struct *vma, struct vm_area_struct **pprev, unsigned long start, unsigned long end, unsigned long newflags) { unsigned long masked_newflags = newflags & (VM_READ | VM_WRITE | VM_EXEC); int ret; ret = sgx_encl_may_map(vma->vm_private_data, start, end, masked_newflags); if (ret) return ret; return mprotect_fixup(vma, pprev, start, end, newflags); } Alternatively the filtering can be inside sgx_encl_may_map(). Perhaps that is a better place for it. This was just easier function to represent the idea. /Jarkko
On Wed, Jul 08, 2020 at 07:10:27PM +0300, Jarkko Sakkinen wrote: > On Wed, Jul 08, 2020 at 03:37:08PM +0100, Matthew Wilcox wrote: > > On Wed, Jul 08, 2020 at 05:33:20PM +0300, Jarkko Sakkinen wrote: > > > I get the point but I don't think that your proposal could work given > > > that mprotect-callback takes neither 'prev' nor 'newflags' as its > > > parameters. The current callback has no means to call mprotect_fixup() > > > properly. > > > > > > It would have to be extended > > > > > > int (*mprotect)(struct vm_area_struct *vma, > > > struct vm_area_struct **pprev, unsigned long start, > > > unsigned long end, unsigned long prot, > > > unsigned long newflags); > > > > > > Is this what you want? > > > > https://lore.kernel.org/linux-mm/20200625173050.GF7703@casper.infradead.org/ > > Ugh, it's there as it should be. I'm sorry - I just misread the code. > > I think that should work, and we do not have to do extra conversion > inside the callback. > > There is still one thing that I'm wondering. 'page->vm_max_prot_bits' > contains some an union of subset of {VM_READ, VM_WRITE, VM_EXEC}, and > 'newflags' can contain other bits set too. > > The old implementation of sgx_vma_mprotect() is like this: > > static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start, > unsigned long end, unsigned long prot) > { > return sgx_encl_may_map(vma->vm_private_data, start, end, > calc_vm_prot_bits(prot, 0)); > } > > The new one should be probably the implemented along the lines of > > static int sgx_vma_mprotect(struct vm_area_struct *vma, > struct vm_area_struct **pprev, unsigned long start, > unsigned long end, unsigned long newflags) > { > unsigned long masked_newflags = newflags & > (VM_READ | VM_WRITE | VM_EXEC); > int ret; > > ret = sgx_encl_may_map(vma->vm_private_data, start, end, > masked_newflags); > if (ret) > return ret; > > return mprotect_fixup(vma, pprev, start, end, newflags); > } > > Alternatively the filtering can be inside sgx_encl_may_map(). Perhaps > that is a better place for it. This was just easier function to > represent the idea. Turned out that mmap() handler was already masking with RWX. So I removed masking from there and do it as the first step in sgx_encl_may_map(), which is called by the both handlers: int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start, unsigned long end, unsigned long vm_flags) { unsigned long vm_prot_bits = vm_flags & (VM_READ | VM_WRITE | VM_EXEC); unsigned long idx, idx_start, idx_end; struct sgx_encl_page *page; Also renamed the last function parameter from vm_flags to vm_port_bits. Kind of makes the flow more understandable (i.e. vm_prot_bits is purely internal representation, not something caller needs to be concerned of). /Jarkko
diff --git a/include/linux/mm.h b/include/linux/mm.h index dc7b87310c10..fc0e3ef28873 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -542,6 +542,8 @@ struct vm_operations_struct { void (*close)(struct vm_area_struct * area); int (*split)(struct vm_area_struct * area, unsigned long addr); int (*mremap)(struct vm_area_struct * area); + int (*mprotect)(struct vm_area_struct *vma, unsigned long start, + unsigned long end, unsigned long prot); vm_fault_t (*fault)(struct vm_fault *vmf); vm_fault_t (*huge_fault)(struct vm_fault *vmf, enum page_entry_size pe_size); diff --git a/mm/mprotect.c b/mm/mprotect.c index ce8b8a5eacbb..e23dfd8d18bc 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -603,13 +603,20 @@ static int do_mprotect_pkey(unsigned long start, size_t len, goto out; } + tmp = vma->vm_end; + if (tmp > end) + tmp = end; + error = security_file_mprotect(vma, reqprot, prot); if (error) goto out; - tmp = vma->vm_end; - if (tmp > end) - tmp = end; + if (vma->vm_ops && vma->vm_ops->mprotect) { + error = vma->vm_ops->mprotect(vma, nstart, tmp, prot); + if (error) + goto out; + } + error = mprotect_fixup(vma, &prev, nstart, tmp, newflags); if (error) goto out;