| Message ID | 20230719075127.47736-4-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: convert to vma_is_initial_heap/stack() | expand |
On 19.07.23 09:51, Kefeng Wang wrote: > Use the helpers to simplify code. > > Cc: Paul Moore <paul@paul-moore.com> > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > Cc: Eric Paris <eparis@parisplace.org> > Acked-by: Paul Moore <paul@paul-moore.com> > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > security/selinux/hooks.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index d06e350fedee..ee8575540a8e 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3762,13 +3762,10 @@ static int selinux_file_mprotect(struct vm_area_struct *vma, > if (default_noexec && > (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { > int rc = 0; > - if (vma->vm_start >= vma->vm_mm->start_brk && > - vma->vm_end <= vma->vm_mm->brk) { > + if (vma_is_initial_heap(vma)) { > rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, > PROCESS__EXECHEAP, NULL); > - } else if (!vma->vm_file && > - ((vma->vm_start <= vma->vm_mm->start_stack && > - vma->vm_end >= vma->vm_mm->start_stack) || > + } else if (!vma->vm_file && (vma_is_initial_stack(vma) || > vma_is_stack_for_current(vma))) { > rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, > PROCESS__EXECSTACK, NULL); Reviewed-by: David Hildenbrand <david@redhat.com>
On Wed, 19 Jul 2023 at 09:40, Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > Use the helpers to simplify code. > > Cc: Paul Moore <paul@paul-moore.com> > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > Cc: Eric Paris <eparis@parisplace.org> > Acked-by: Paul Moore <paul@paul-moore.com> > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > security/selinux/hooks.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index d06e350fedee..ee8575540a8e 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3762,13 +3762,10 @@ static int selinux_file_mprotect(struct vm_area_struct *vma, > if (default_noexec && > (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { > int rc = 0; > - if (vma->vm_start >= vma->vm_mm->start_brk && > - vma->vm_end <= vma->vm_mm->brk) { > + if (vma_is_initial_heap(vma)) { This seems to change the condition from vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <= vma->vm_mm->brk to vma->vm_start <= vma->vm_mm->brk && vma->vm_end >= vma->vm_mm->start_brk (or AND arguments swapped) vma->vm_end >= vma->vm_mm->start_brk && vma->vm_start <= vma->vm_mm->brk Is this intended? > rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, > PROCESS__EXECHEAP, NULL); > - } else if (!vma->vm_file && > - ((vma->vm_start <= vma->vm_mm->start_stack && > - vma->vm_end >= vma->vm_mm->start_stack) || > + } else if (!vma->vm_file && (vma_is_initial_stack(vma) || > vma_is_stack_for_current(vma))) { > rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, > PROCESS__EXECSTACK, NULL); > -- > 2.27.0 >
On 2023/7/19 17:02, Christian Göttsche wrote: > On Wed, 19 Jul 2023 at 09:40, Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >> >> Use the helpers to simplify code. >> >> Cc: Paul Moore <paul@paul-moore.com> >> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> >> Cc: Eric Paris <eparis@parisplace.org> >> Acked-by: Paul Moore <paul@paul-moore.com> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> security/selinux/hooks.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index d06e350fedee..ee8575540a8e 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -3762,13 +3762,10 @@ static int selinux_file_mprotect(struct vm_area_struct *vma, >> if (default_noexec && >> (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { >> int rc = 0; >> - if (vma->vm_start >= vma->vm_mm->start_brk && >> - vma->vm_end <= vma->vm_mm->brk) { >> + if (vma_is_initial_heap(vma)) { > > This seems to change the condition from > > vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <= vma->vm_mm->brk > > to > > vma->vm_start <= vma->vm_mm->brk && vma->vm_end >= vma->vm_mm->start_brk > > (or AND arguments swapped) > > vma->vm_end >= vma->vm_mm->start_brk && vma->vm_start <= vma->vm_mm->brk > > Is this intended? The new condition is to check whether there is intersection between [startbrk,brk] and [vm_start,vm_end], it contains orignal check, so I think it is ok, but for selinux check, I am not sure if there is some other problem. > >> rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, >> PROCESS__EXECHEAP, NULL); >> - } else if (!vma->vm_file && >> - ((vma->vm_start <= vma->vm_mm->start_stack && >> - vma->vm_end >= vma->vm_mm->start_stack) || >> + } else if (!vma->vm_file && (vma_is_initial_stack(vma) || >> vma_is_stack_for_current(vma))) { >> rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, >> PROCESS__EXECSTACK, NULL); >> -- >> 2.27.0 >>
On Wed, Jul 19, 2023 at 6:23 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > On 2023/7/19 17:02, Christian Göttsche wrote: > > On Wed, 19 Jul 2023 at 09:40, Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> > >> Use the helpers to simplify code. > >> > >> Cc: Paul Moore <paul@paul-moore.com> > >> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > >> Cc: Eric Paris <eparis@parisplace.org> > >> Acked-by: Paul Moore <paul@paul-moore.com> > >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > >> --- > >> security/selinux/hooks.c | 7 ++----- > >> 1 file changed, 2 insertions(+), 5 deletions(-) > >> > >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >> index d06e350fedee..ee8575540a8e 100644 > >> --- a/security/selinux/hooks.c > >> +++ b/security/selinux/hooks.c > >> @@ -3762,13 +3762,10 @@ static int selinux_file_mprotect(struct vm_area_struct *vma, > >> if (default_noexec && > >> (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { > >> int rc = 0; > >> - if (vma->vm_start >= vma->vm_mm->start_brk && > >> - vma->vm_end <= vma->vm_mm->brk) { > >> + if (vma_is_initial_heap(vma)) { > > > > This seems to change the condition from > > > > vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <= vma->vm_mm->brk > > > > to > > > > vma->vm_start <= vma->vm_mm->brk && vma->vm_end >= vma->vm_mm->start_brk > > > > (or AND arguments swapped) > > > > vma->vm_end >= vma->vm_mm->start_brk && vma->vm_start <= vma->vm_mm->brk > > > > Is this intended? > > The new condition is to check whether there is intersection between > [startbrk,brk] and [vm_start,vm_end], it contains orignal check, so > I think it is ok, but for selinux check, I am not sure if there is > some other problem. This particular SELinux vma check is see if the vma falls within the heap; can you confirm that this change preserves this?
On 2023/7/19 23:25, Paul Moore wrote: > On Wed, Jul 19, 2023 at 6:23 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >> On 2023/7/19 17:02, Christian Göttsche wrote: >>> On Wed, 19 Jul 2023 at 09:40, Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >>>> >>>> Use the helpers to simplify code. >>>> >>>> Cc: Paul Moore <paul@paul-moore.com> >>>> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> >>>> Cc: Eric Paris <eparis@parisplace.org> >>>> Acked-by: Paul Moore <paul@paul-moore.com> >>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>>> --- >>>> security/selinux/hooks.c | 7 ++----- >>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>> index d06e350fedee..ee8575540a8e 100644 >>>> --- a/security/selinux/hooks.c >>>> +++ b/security/selinux/hooks.c >>>> @@ -3762,13 +3762,10 @@ static int selinux_file_mprotect(struct vm_area_struct *vma, >>>> if (default_noexec && >>>> (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { >>>> int rc = 0; >>>> - if (vma->vm_start >= vma->vm_mm->start_brk && >>>> - vma->vm_end <= vma->vm_mm->brk) { >>>> + if (vma_is_initial_heap(vma)) { >>> >>> This seems to change the condition from >>> >>> vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <= vma->vm_mm->brk >>> >>> to >>> >>> vma->vm_start <= vma->vm_mm->brk && vma->vm_end >= vma->vm_mm->start_brk >>> >>> (or AND arguments swapped) >>> >>> vma->vm_end >= vma->vm_mm->start_brk && vma->vm_start <= vma->vm_mm->brk >>> >>> Is this intended? >> >> The new condition is to check whether there is intersection between >> [startbrk,brk] and [vm_start,vm_end], it contains orignal check, so >> I think it is ok, but for selinux check, I am not sure if there is >> some other problem. > > This particular SELinux vma check is see if the vma falls within the > heap; can you confirm that this change preserves this? Yes, within is one case of new vma scope check. >
On Thu, Jul 20, 2023 at 4:28 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > On 2023/7/19 23:25, Paul Moore wrote: > > On Wed, Jul 19, 2023 at 6:23 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> On 2023/7/19 17:02, Christian Göttsche wrote: > >>> On Wed, 19 Jul 2023 at 09:40, Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >>>> > >>>> Use the helpers to simplify code. > >>>> > >>>> Cc: Paul Moore <paul@paul-moore.com> > >>>> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > >>>> Cc: Eric Paris <eparis@parisplace.org> > >>>> Acked-by: Paul Moore <paul@paul-moore.com> > >>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > >>>> --- > >>>> security/selinux/hooks.c | 7 ++----- > >>>> 1 file changed, 2 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >>>> index d06e350fedee..ee8575540a8e 100644 > >>>> --- a/security/selinux/hooks.c > >>>> +++ b/security/selinux/hooks.c > >>>> @@ -3762,13 +3762,10 @@ static int selinux_file_mprotect(struct vm_area_struct *vma, > >>>> if (default_noexec && > >>>> (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { > >>>> int rc = 0; > >>>> - if (vma->vm_start >= vma->vm_mm->start_brk && > >>>> - vma->vm_end <= vma->vm_mm->brk) { > >>>> + if (vma_is_initial_heap(vma)) { > >>> > >>> This seems to change the condition from > >>> > >>> vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <= vma->vm_mm->brk > >>> > >>> to > >>> > >>> vma->vm_start <= vma->vm_mm->brk && vma->vm_end >= vma->vm_mm->start_brk > >>> > >>> (or AND arguments swapped) > >>> > >>> vma->vm_end >= vma->vm_mm->start_brk && vma->vm_start <= vma->vm_mm->brk > >>> > >>> Is this intended? > >> > >> The new condition is to check whether there is intersection between > >> [startbrk,brk] and [vm_start,vm_end], it contains orignal check, so > >> I think it is ok, but for selinux check, I am not sure if there is > >> some other problem. > > > > This particular SELinux vma check is see if the vma falls within the > > heap; can you confirm that this change preserves this? > > Yes, within is one case of new vma scope check. Thanks for the confirmation.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d06e350fedee..ee8575540a8e 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3762,13 +3762,10 @@ static int selinux_file_mprotect(struct vm_area_struct *vma, if (default_noexec && (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { int rc = 0; - if (vma->vm_start >= vma->vm_mm->start_brk && - vma->vm_end <= vma->vm_mm->brk) { + if (vma_is_initial_heap(vma)) { rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, PROCESS__EXECHEAP, NULL); - } else if (!vma->vm_file && - ((vma->vm_start <= vma->vm_mm->start_stack && - vma->vm_end >= vma->vm_mm->start_stack) || + } else if (!vma->vm_file && (vma_is_initial_stack(vma) || vma_is_stack_for_current(vma))) { rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, PROCESS__EXECSTACK, NULL);