diff mbox series

[RFC,1/4] lsm: Clarify documentation of vm_enough_memory hook

Message ID 20221115175652.3836811-2-roberto.sassu@huaweicloud.com
State New
Headers
Series security: Ensure LSMs return expected values |

Commit Message

Roberto Sassu Nov. 15, 2022, 5:56 p.m. UTC 
  From: Roberto Sassu <roberto.sassu@huawei.com>

include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
the callers, not what LSMs should return to the LSM infrastructure.

Clarify that and add that returning 1 from the LSMs means calling
__vm_enough_memory() with cap_sys_admin set, 0 without.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/lsm_hooks.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Paul Moore Nov. 16, 2022, 2:11 a.m. UTC | #1 
On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
> the callers, not what LSMs should return to the LSM infrastructure.
>
> Clarify that and add that returning 1 from the LSMs means calling
> __vm_enough_memory() with cap_sys_admin set, 0 without.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: KP Singh <kpsingh@kernel.org>
> ---
>  include/linux/lsm_hooks.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4ec80b96c22e..f40b82ca91e7 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1411,7 +1411,9 @@
>   *     Check permissions for allocating a new virtual mapping.
>   *     @mm contains the mm struct it is being added to.
>   *     @pages contains the number of pages.
> - *     Return 0 if permission is granted.
> + *     Return 0 if permission is granted by LSMs to the caller. LSMs should
> + *     return 1 if __vm_enough_memory() should be called with
> + *     cap_sys_admin set, 0 if not.

I think this is a nice addition, but according to the code, any value
greater than zero will trigger the caller-should-have-CAP_SYS_ADMIN
behavior, not just 1.  I suggest updating the comment.
Roberto Sassu Nov. 16, 2022, 8:06 a.m. UTC | #2 
On Tue, 2022-11-15 at 21:11 -0500, Paul Moore wrote:
> On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
> > the callers, not what LSMs should return to the LSM infrastructure.
> > 
> > Clarify that and add that returning 1 from the LSMs means calling
> > __vm_enough_memory() with cap_sys_admin set, 0 without.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  include/linux/lsm_hooks.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 4ec80b96c22e..f40b82ca91e7 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1411,7 +1411,9 @@
> >   *     Check permissions for allocating a new virtual mapping.
> >   *     @mm contains the mm struct it is being added to.
> >   *     @pages contains the number of pages.
> > - *     Return 0 if permission is granted.
> > + *     Return 0 if permission is granted by LSMs to the caller. LSMs should
> > + *     return 1 if __vm_enough_memory() should be called with
> > + *     cap_sys_admin set, 0 if not.
> 
> I think this is a nice addition, but according to the code, any value
> greater than zero will trigger the caller-should-have-CAP_SYS_ADMIN
> behavior, not just 1.  I suggest updating the comment.

Ok, yes. Thanks.

Roberto
KP Singh Nov. 16, 2022, 7:17 p.m. UTC | #3 
On Wed, Nov 16, 2022 at 9:06 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Tue, 2022-11-15 at 21:11 -0500, Paul Moore wrote:
> > On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
> > > the callers, not what LSMs should return to the LSM infrastructure.
> > >
> > > Clarify that and add that returning 1 from the LSMs means calling
> > > __vm_enough_memory() with cap_sys_admin set, 0 without.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > > ---
> > >  include/linux/lsm_hooks.h | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > > index 4ec80b96c22e..f40b82ca91e7 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1411,7 +1411,9 @@
> > >   *     Check permissions for allocating a new virtual mapping.
> > >   *     @mm contains the mm struct it is being added to.
> > >   *     @pages contains the number of pages.
> > > - *     Return 0 if permission is granted.
> > > + *     Return 0 if permission is granted by LSMs to the caller. LSMs should
> > > + *     return 1 if __vm_enough_memory() should be called with
> > > + *     cap_sys_admin set, 0 if not.
> >
> > I think this is a nice addition, but according to the code, any value
> > greater than zero will trigger the caller-should-have-CAP_SYS_ADMIN
> > behavior, not just 1.  I suggest updating the comment.
>
> Ok, yes. Thanks.

Also, this is an unrelated patch and you can probably send it
independently, especially
since the other changes will now land mostly via BPF.

>
> Roberto
>
Paul Moore Nov. 16, 2022, 7:27 p.m. UTC | #4 
On Wed, Nov 16, 2022 at 2:18 PM KP Singh <kpsingh@kernel.org> wrote:
> On Wed, Nov 16, 2022 at 9:06 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> >
> > On Tue, 2022-11-15 at 21:11 -0500, Paul Moore wrote:
> > > On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > >
> > > > include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
> > > > the callers, not what LSMs should return to the LSM infrastructure.
> > > >
> > > > Clarify that and add that returning 1 from the LSMs means calling
> > > > __vm_enough_memory() with cap_sys_admin set, 0 without.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > > > ---
> > > >  include/linux/lsm_hooks.h | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > > > index 4ec80b96c22e..f40b82ca91e7 100644
> > > > --- a/include/linux/lsm_hooks.h
> > > > +++ b/include/linux/lsm_hooks.h
> > > > @@ -1411,7 +1411,9 @@
> > > >   *     Check permissions for allocating a new virtual mapping.
> > > >   *     @mm contains the mm struct it is being added to.
> > > >   *     @pages contains the number of pages.
> > > > - *     Return 0 if permission is granted.
> > > > + *     Return 0 if permission is granted by LSMs to the caller. LSMs should
> > > > + *     return 1 if __vm_enough_memory() should be called with
> > > > + *     cap_sys_admin set, 0 if not.
> > >
> > > I think this is a nice addition, but according to the code, any value
> > > greater than zero will trigger the caller-should-have-CAP_SYS_ADMIN
> > > behavior, not just 1.  I suggest updating the comment.
> >
> > Ok, yes. Thanks.
>
> Also, this is an unrelated patch and you can probably send it
> independently, especially
> since the other changes will now land mostly via BPF.

Yes, the doc/comment changes really have nothing to do with the other
stuff we are discussing in this patchset.
diff mbox series

Patch

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 4ec80b96c22e..f40b82ca91e7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1411,7 +1411,9 @@ 
  *	Check permissions for allocating a new virtual mapping.
  *	@mm contains the mm struct it is being added to.
  *	@pages contains the number of pages.
- *	Return 0 if permission is granted.
+ *	Return 0 if permission is granted by LSMs to the caller. LSMs should
+ *	return 1 if __vm_enough_memory() should be called with
+ *	cap_sys_admin set, 0 if not.
  *
  * @ismaclabel:
  *	Check if the extended attribute specified by @name