mbox series

[v1,0/2] Refactor return value of two lsm hooks

Message ID 20240724020659.120353-1-xukuohai@huaweicloud.com (mailing list archive)
Headers show
Series Refactor return value of two lsm hooks | expand

Message

Xu Kuohai July 24, 2024, 2:06 a.m. UTC
From: Xu Kuohai <xukuohai@huawei.com>

The BPF LSM program may cause a kernel panic if it returns an
unexpected value, such as a positive value on the hook
file_alloc_security.

To fix it, series [1] refactored the LSM hook return values and
added BPF return value checks.

[1] used two methods to refactor hook return values:

- converting positive return value to negative error code

- adding additional output parameter to store odd return values

Based on discussion in [1], only two hooks refactored with the
second method may be acceptable. Since the second method requires
extra work on BPF side to ensure that the output parameter is
set properly, the extra work does not seem worthwhile for just
two hooks. So this series includes only the two patches refactored
with the first method.

Changes to [1]:
- Drop unnecessary patches
- Rebase
- Remove redundant comments in the inode_copy_up_xattr patch

[1] https://lore.kernel.org/bpf/20240711111908.3817636-1-xukuohai@huaweicloud.com
    https://lore.kernel.org/bpf/20240711113828.3818398-1-xukuohai@huaweicloud.com

Xu Kuohai (2):
  lsm: Refactor return value of LSM hook vm_enough_memory
  lsm: Refactor return value of LSM hook inode_copy_up_xattr

 fs/overlayfs/copy_up.c            |  6 +++---
 include/linux/lsm_hook_defs.h     |  2 +-
 include/linux/security.h          |  2 +-
 security/commoncap.c              | 11 +++--------
 security/integrity/evm/evm_main.c |  2 +-
 security/security.c               | 22 ++++++++--------------
 security/selinux/hooks.c          | 19 ++++++-------------
 security/smack/smack_lsm.c        |  6 +++---
 8 files changed, 26 insertions(+), 44 deletions(-)

Comments

Casey Schaufler July 24, 2024, 8:36 p.m. UTC | #1
On 7/23/2024 7:06 PM, Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
>
> The BPF LSM program may cause a kernel panic if it returns an
> unexpected value, such as a positive value on the hook
> file_alloc_security.
>
> To fix it, series [1] refactored the LSM hook return values and
> added BPF return value checks.
>
> [1] used two methods to refactor hook return values:
>
> - converting positive return value to negative error code
>
> - adding additional output parameter to store odd return values
>
> Based on discussion in [1], only two hooks refactored with the
> second method may be acceptable. Since the second method requires
> extra work on BPF side to ensure that the output parameter is
> set properly, the extra work does not seem worthwhile for just
> two hooks. So this series includes only the two patches refactored
> with the first method.
>
> Changes to [1]:
> - Drop unnecessary patches
> - Rebase
> - Remove redundant comments in the inode_copy_up_xattr patch
>
> [1] https://lore.kernel.org/bpf/20240711111908.3817636-1-xukuohai@huaweicloud.com
>     https://lore.kernel.org/bpf/20240711113828.3818398-1-xukuohai@huaweicloud.com
>
> Xu Kuohai (2):
>   lsm: Refactor return value of LSM hook vm_enough_memory
>   lsm: Refactor return value of LSM hook inode_copy_up_xattr

For the series:
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>

>
>  fs/overlayfs/copy_up.c            |  6 +++---
>  include/linux/lsm_hook_defs.h     |  2 +-
>  include/linux/security.h          |  2 +-
>  security/commoncap.c              | 11 +++--------
>  security/integrity/evm/evm_main.c |  2 +-
>  security/security.c               | 22 ++++++++--------------
>  security/selinux/hooks.c          | 19 ++++++-------------
>  security/smack/smack_lsm.c        |  6 +++---
>  8 files changed, 26 insertions(+), 44 deletions(-)
>
Paul Moore July 24, 2024, 9:55 p.m. UTC | #2
On Wed, Jul 24, 2024 at 4:36 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/23/2024 7:06 PM, Xu Kuohai wrote:
> > From: Xu Kuohai <xukuohai@huawei.com>
> >
> > The BPF LSM program may cause a kernel panic if it returns an
> > unexpected value, such as a positive value on the hook
> > file_alloc_security.
> >
> > To fix it, series [1] refactored the LSM hook return values and
> > added BPF return value checks.
> >
> > [1] used two methods to refactor hook return values:
> >
> > - converting positive return value to negative error code
> >
> > - adding additional output parameter to store odd return values
> >
> > Based on discussion in [1], only two hooks refactored with the
> > second method may be acceptable. Since the second method requires
> > extra work on BPF side to ensure that the output parameter is
> > set properly, the extra work does not seem worthwhile for just
> > two hooks. So this series includes only the two patches refactored
> > with the first method.
> >
> > Changes to [1]:
> > - Drop unnecessary patches
> > - Rebase
> > - Remove redundant comments in the inode_copy_up_xattr patch
> >
> > [1] https://lore.kernel.org/bpf/20240711111908.3817636-1-xukuohai@huaweicloud.com
> >     https://lore.kernel.org/bpf/20240711113828.3818398-1-xukuohai@huaweicloud.com
> >
> > Xu Kuohai (2):
> >   lsm: Refactor return value of LSM hook vm_enough_memory
> >   lsm: Refactor return value of LSM hook inode_copy_up_xattr
>
> For the series:
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>

Looks good to me too.  I'm going to merge this into lsm/dev-staging
for testing with the expectation that I'll move them over to lsm/dev
once the merge window closes.

Thanks!
Paul Moore July 29, 2024, 9:19 p.m. UTC | #3
On Wed, Jul 24, 2024 at 5:55 PM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Jul 24, 2024 at 4:36 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 7/23/2024 7:06 PM, Xu Kuohai wrote:
> > > From: Xu Kuohai <xukuohai@huawei.com>
> > >
> > > The BPF LSM program may cause a kernel panic if it returns an
> > > unexpected value, such as a positive value on the hook
> > > file_alloc_security.
> > >
> > > To fix it, series [1] refactored the LSM hook return values and
> > > added BPF return value checks.
> > >
> > > [1] used two methods to refactor hook return values:
> > >
> > > - converting positive return value to negative error code
> > >
> > > - adding additional output parameter to store odd return values
> > >
> > > Based on discussion in [1], only two hooks refactored with the
> > > second method may be acceptable. Since the second method requires
> > > extra work on BPF side to ensure that the output parameter is
> > > set properly, the extra work does not seem worthwhile for just
> > > two hooks. So this series includes only the two patches refactored
> > > with the first method.
> > >
> > > Changes to [1]:
> > > - Drop unnecessary patches
> > > - Rebase
> > > - Remove redundant comments in the inode_copy_up_xattr patch
> > >
> > > [1] https://lore.kernel.org/bpf/20240711111908.3817636-1-xukuohai@huaweicloud.com
> > >     https://lore.kernel.org/bpf/20240711113828.3818398-1-xukuohai@huaweicloud.com
> > >
> > > Xu Kuohai (2):
> > >   lsm: Refactor return value of LSM hook vm_enough_memory
> > >   lsm: Refactor return value of LSM hook inode_copy_up_xattr
> >
> > For the series:
> > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
>
> Looks good to me too.  I'm going to merge this into lsm/dev-staging
> for testing with the expectation that I'll move them over to lsm/dev
> once the merge window closes.

These patches are now lsm/dev, thanks again for your help on this patchset.