Re: [PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()

2023-07-21 Thread Paul Moore 
On Thu, Jul 20, 2023 at 4:28 AM Kefeng Wang  wrote:
> On 2023/7/19 23:25, Paul Moore wrote:
> > On Wed, Jul 19, 2023 at 6:23 AM Kefeng Wang  
> > wrote:
> >> On 2023/7/19 17:02, Christian Göttsche wrote:
> >>> On Wed, 19 Jul 2023 at 09:40, Kefeng Wang  
> >>> wrote:
> 
>  Use the helpers to simplify code.
> 
>  Cc: Paul Moore 
>  Cc: Stephen Smalley 
>  Cc: Eric Paris 
>  Acked-by: Paul Moore 
>  Signed-off-by: Kefeng Wang 
>  ---
> 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.

-- 
paul-moore.com

Re: [PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()

2023-07-20 Thread Paul Moore 
On Wed, Jul 19, 2023 at 6:23 AM Kefeng Wang  wrote:
> On 2023/7/19 17:02, Christian Göttsche wrote:
> > On Wed, 19 Jul 2023 at 09:40, Kefeng Wang  
> > wrote:
> >>
> >> Use the helpers to simplify code.
> >>
> >> Cc: Paul Moore 
> >> Cc: Stephen Smalley 
> >> Cc: Eric Paris 
> >> Acked-by: Paul Moore 
> >> Signed-off-by: Kefeng Wang 
> >> ---
> >>   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?

-- 
paul-moore.com

Re: [PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()

2023-07-20 Thread Christian Göttsche 
On Wed, 19 Jul 2023 at 09:40, Kefeng Wang  wrote:
>
> Use the helpers to simplify code.
>
> Cc: Paul Moore 
> Cc: Stephen Smalley 
> Cc: Eric Paris 
> Acked-by: Paul Moore 
> Signed-off-by: Kefeng Wang 
> ---
>  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
>

Re: [PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()

2023-07-20 Thread Kefeng Wang 




On 2023/7/19 23:25, Paul Moore wrote:

On Wed, Jul 19, 2023 at 6:23 AM Kefeng Wang  wrote:

On 2023/7/19 17:02, Christian Göttsche wrote:

On Wed, 19 Jul 2023 at 09:40, Kefeng Wang  wrote:


Use the helpers to simplify code.

Cc: Paul Moore 
Cc: Stephen Smalley 
Cc: Eric Paris 
Acked-by: Paul Moore 
Signed-off-by: Kefeng Wang 
---
   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.

Re: [PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()

2023-07-19 Thread Kefeng Wang 




On 2023/7/19 17:02, Christian Göttsche wrote:

On Wed, 19 Jul 2023 at 09:40, Kefeng Wang  wrote:


Use the helpers to simplify code.

Cc: Paul Moore 
Cc: Stephen Smalley 
Cc: Eric Paris 
Acked-by: Paul Moore 
Signed-off-by: Kefeng Wang 
---
  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

Re: [PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()

2023-07-19 Thread David Hildenbrand 

On 19.07.23 09:51, Kefeng Wang wrote:

Use the helpers to simplify code.

Cc: Paul Moore 
Cc: Stephen Smalley 
Cc: Eric Paris 
Acked-by: Paul Moore 
Signed-off-by: Kefeng Wang 
---
  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 

--
Cheers,

David / dhildenb

[PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()

2023-07-19 Thread Kefeng Wang 
Use the helpers to simplify code.

Cc: Paul Moore 
Cc: Stephen Smalley 
Cc: Eric Paris 
Acked-by: Paul Moore 
Signed-off-by: Kefeng Wang 
---
 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);
-- 
2.27.0