diff mbox

[09/10] mm: Prevent madvise from changing shadow stack

Message ID 20180607143807.3611-10-yu-cheng.yu@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yu-cheng Yu June 7, 2018, 2:38 p.m. UTC 
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 mm/madvise.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Andy Lutomirski June 7, 2018, 8:54 p.m. UTC | #1 
On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>

Seems reasonable to me.
Nadav Amit June 7, 2018, 9:09 p.m. UTC | #2 
Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:

> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> mm/madvise.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 4d3c922ea1a1..2a6988badd6b 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -839,6 +839,14 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> 	if (vma && start > vma->vm_start)
> 		prev = vma;
> 
> +	/*
> +	 * Don't do anything on shadow stack.
> +	 */
> +	if (vma->vm_flags & VM_SHSTK) {
> +		error = -EINVAL;
> +		goto out_no_plug;
> +	}
> +
> 	blk_start_plug(&plug);
> 	for (;;) {
> 		/* Still start < end. */

What happens if the madvise() revolves multiple VMAs, the first one is not
VM_SHSTK, but the another one is? Shouldn’t the test be done inside the
loop, potentially in madvise_vma() ?
Yu-cheng Yu June 7, 2018, 9:18 p.m. UTC | #3 
On Thu, 2018-06-07 at 14:09 -0700, Nadav Amit wrote:
> Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> 
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > ---
> > mm/madvise.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> > 
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 4d3c922ea1a1..2a6988badd6b 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -839,6 +839,14 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > 	if (vma && start > vma->vm_start)
> > 		prev = vma;
> > 
> > +	/*
> > +	 * Don't do anything on shadow stack.
> > +	 */
> > +	if (vma->vm_flags & VM_SHSTK) {
> > +		error = -EINVAL;
> > +		goto out_no_plug;
> > +	}
> > +
> > 	blk_start_plug(&plug);
> > 	for (;;) {
> > 		/* Still start < end. */
> 
> What happens if the madvise() revolves multiple VMAs, the first one is not
> VM_SHSTK, but the another one is? Shouldn’t the test be done inside the
> loop, potentially in madvise_vma() ?
> 

I will fix it.  Thanks!
diff mbox

Patch

diff --git a/mm/madvise.c b/mm/madvise.c
index 4d3c922ea1a1..2a6988badd6b 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -839,6 +839,14 @@  SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 	if (vma && start > vma->vm_start)
 		prev = vma;
 
+	/*
+	 * Don't do anything on shadow stack.
+	 */
+	if (vma->vm_flags & VM_SHSTK) {
+		error = -EINVAL;
+		goto out_no_plug;
+	}
+
 	blk_start_plug(&plug);
 	for (;;) {
 		/* Still start < end. */
@@ -876,6 +884,7 @@  SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 	}
 out:
 	blk_finish_plug(&plug);
+out_no_plug:
 	if (write)
 		up_write(&current->mm->mmap_sem);
 	else