[GIT,PULL] apparmor changes for v6.2

  Hi Linus,

Please pull the following apparmor update for Linux 6.2-rc1

This apparmor update for 6.2 is primarily bug fixes and code
cleanup/refactoring.

+ Features
   - switch to zstd compression for profile raw data

+ Cleanups
   - Simplify obtain the newest label on a cred
   - remove useless static inline functions
   - compute permission conversion on policy unpack
   - refactor code to share common permissins
   - refactor unpack to group policy backwards compatiblity code
   - add __init annotation to aa_{setup/teardown}_dfa_engine()

+ Bug Fixes
   - fix a memleak in
     - multi_transaction_new()
     - free_ruleset()
     - unpack_profile()
     - alloc_ns()
   - fix lockdep warning when removing a namespace
   - fix regression in stacking due to label flags
   - fix loading of child before parent
   - fix kernel-doc comments that differ from fns
   - fix spelling errors in comments
   - store return value of unpack_perms_table() to signed variable


The bulk of the code is in a set of changes is a cleanup that moves
some permission computations from run time to policy is load. This
provides a small performance improvement but is also a step toward
us being able to merge and reduce the different permission lookup
code at runtime.


Please note this patch series has conflicts with the KUnit and LSM trees,
and has been coordinated to follow those merges as all the conflicts are
in apparmor

- The LSM tree conflict is a simple conflict in
     security/apparmor/domain.c
   between the apparmor permission cleanups and
     f6fbd8cbf3ed lsm,fs: fix vfs_getxattr_alloc() return type and caller error paths

- The KUnit tree conflict is large but fairly straight forward. It is
   large due to the code refactoring in the permissions cleanup, so it
   covers some code that does not get marked as having conflicts.
   The gist of the conflicting Kunit change is that to be able to make
   the apparmor KUnit tests as module several apparmor symbols that
   were previously static need to be exported. To do this the symbols
   are prefixed with the aa_ namespace and conditionally exported with
   VISIBLE_IF_KUNIT

   conflicts in
     security/apparmor/policy_unpack.c
     security/apparmor/policy_unpack_test.c

   with additional changes beyond the conflicts needed in
     security/apparmor/policy_unpack.c
     security/apparmor/include/policy_unpack.h

I have prepared two branches covering the merge conflict.


The first
   git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2022-12-14-merge-breakout

branch breaks the changes into 3 patches.
- the merge and any changes that had associated conflicts
- a patch where Stephen Rothwell beat me to fixing a build failure
   in linux-next due to the merge needing changes beyond those with
   associated conflicts
- a patch covering the rest of changes that are required but did not
   have conflicts and are not covered by Stephen's patch.


The second
   git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2022-12-14-merge

is all the changes from the first as a single merge commit


the above merge conflict resolution has been built and tested
with Kunit as built-in, module and disabled.

thanks
- john


The following changes since commit 3cc40a443a04d52b0c95255dce264068b01e9bfe:

   Merge tag 'nios2_fixes_v6.0' of git://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux (2022-08-16 11:42:11 -0700)

are available in the Git repository at:

   git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2022-12-14

for you to fetch changes up to 4295c60bbe9e63e35d330546eeaa1d2b62dae303:

   apparmor: Fix uninitialized symbol 'array_size' in policy_unpack_test.c (2022-11-01 21:23:05 -0700)

----------------------------------------------------------------
+ Features
   - switch to zstd compression for profile raw data

+ Cleanups
   - Simplify obtain the newest label on a cred
   - remove useless static inline functions
   - compute permission conversion on policy unpack
   - refactor code to share common permissins
   - refactor unpack to group policy backwards compatiblity code
   - add __init annotation to aa_{setup/teardown}_dfa_engine()

+ Bug Fixes
   - fix a memleak in
     - multi_transaction_new()
     - free_ruleset()
     - unpack_profile()
     - alloc_ns()
   - fix lockdep warning when removing a namespace
   - fix regression in stacking due to label flags
   - fix loading of child before parent
   - fix kernel-doc comments that differ from fns
   - fix spelling errors in comments
   - store return value of unpack_perms_table() to signed variable

----------------------------------------------------------------
Gaosheng Cui (5):
       apparmor: fix a memleak in multi_transaction_new()
       apparmor: Simplify obtain the newest label on a cred
       apparmor: remove useless static inline functions
       apparmor: fix obsoleted comments for aa_getprocattr() and audit_resource()
       apparmor: fix a memleak in free_ruleset()

Jiapeng Chong (3):
       AppArmor: Fix kernel-doc
       LSM: Fix kernel-doc
       AppArmor: Fix kernel-doc

John Johansen (45):
       apparmor: fix lockdep warning when removing a namespace
       apparmor: reserve mediation classes
       apparmor: move fperm computation into policy_unpack
       apparmor: rework and cleanup fperm computation
       apparmor: convert xmatch to use aa_perms structure
       apparmor: compute policydb permission on profile load
       apparmor: combine file_rules and aa_policydb into a single shared struct
       apparmor: convert xmatch to using the new shared policydb struct
       apparmor: convert fperm lookup to use accept as an index
       apparmor: convert xmatch lookup to use accept as an index
       apparmor: cleanup shared permission struct
       apparmor: convert policy lookup to use accept as an index
       apparmor: preparse for state being more than just an integer
       apparmor: Fix abi check to include v8 abi
       apparmor: fix apparmor mediating locking non-fs unix sockets
       apparmor: extend policydb permission set by making use of the xbits
       apparmor: move dfa perm macros into policy_unpack
       apparmor: extend xindex size
       apparmor: isolate policy backwards compatibility to its own file
       apparmor: extend permissions to support a label and tag string
       apparmor: add mediation class information to auditing
       apparmor: add user mode flag
       apparmor: make transition table unpack generic so it can be reused
       apparmor: group dfa policydb unpacking
       apparmor: make unpack_array return a trianary value
       apparmor: add the ability for policy to specify a permission table
       apparmor: verify permission table indexes
       apparmor: make sure perm indexes are accumulated
       apparmor: cleanup: move perm accumulation into perms.h
       apparmor: verify loaded permission bits masks don't overlap
       apparmor: refactor profile rules and attachments
       apparmor: rework profile->rules to be a list
       apparmor: fix aa_class_names[] to match reserved classes
       apparmor: Fix regression in stacking due to label flags
       apparmor: Fix doc comment for compute_fperms
       apparmor: Remove unnecessary size check when unpacking trans_table
       apparmor: make sure the decompression ctx is promperly initialized
       apparmor: Fix undefined references to zstd_ symbols
       apparmor: Fix decompression of rawdata for read back to userspace
       apparmor: Fix kunit test for out of bounds array
       apparmor: fix uninitialize table variable in error in unpack_trans_table
       apparmor: Fix unpack_profile() warn: passing zero to 'ERR_PTR'
       apparmor: refactor code that alloc null profiles
       apparmor: Fix loading of child before parent
       apparmor: Fix uninitialized symbol 'array_size' in policy_unpack_test.c

Jon Tourville (2):
       apparmor: use zstd compression for profile data
       apparmor: expose compression level limits in sysfs

Mike Salvatore (2):
       apparmor: compute file permissions on profile load
       apparmor: compute xmatch permissions on profile load

Muhammad Usama Anjum (1):
       apparmor: store return value of unpack_perms_table() to signed variable

Xiu Jianfeng (5):
       apparmor: make __aa_path_perm() static
       apparmor: Use pointer to struct aa_label for lbs_cred
       apparmor: Fix memleak issue in unpack_profile()
       apparmor: Fix memleak in alloc_ns()
       apparmor: Add __init annotation to aa_{setup/teardown}_dfa_engine()

Yang Li (1):
       apparmor: Fix spelling of function name in comment block

  security/apparmor/Kconfig                 |   4 +-
  security/apparmor/Makefile                |   3 +-
  security/apparmor/apparmorfs.c            | 115 +++---
  security/apparmor/audit.c                 |  45 ++-
  security/apparmor/capability.c            |  16 +-
  security/apparmor/domain.c                | 149 ++++----
  security/apparmor/file.c                  | 125 ++-----
  security/apparmor/include/apparmor.h      |  23 +-
  security/apparmor/include/audit.h         |   8 +-
  security/apparmor/include/cred.h          |  13 -
  security/apparmor/include/domain.h        |   6 -
  security/apparmor/include/file.h          | 108 +-----
  security/apparmor/include/label.h         |  13 +-
  security/apparmor/include/lib.h           |  10 +-
  security/apparmor/include/match.h         |  28 +-
  security/apparmor/include/net.h           |   1 +
  security/apparmor/include/perms.h         |  91 ++++-
  security/apparmor/include/policy.h        | 141 +++++--
  security/apparmor/include/policy_compat.h |  33 ++
  security/apparmor/include/policy_unpack.h |   2 +
  security/apparmor/ipc.c                   |  16 +-
  security/apparmor/label.c                 |  75 ++--
  security/apparmor/lib.c                   | 125 ++-----
  security/apparmor/lsm.c                   |  25 +-
  security/apparmor/match.c                 |  62 ++--
  security/apparmor/mount.c                 |  93 +++--
  security/apparmor/net.c                   |  28 +-
  security/apparmor/policy.c                | 204 ++++++++--
  security/apparmor/policy_compat.c         | 319 ++++++++++++++++
  security/apparmor/policy_ns.c             |   6 +-
  security/apparmor/policy_unpack.c         | 598 ++++++++++++++++++++----------
  security/apparmor/policy_unpack_test.c    |  17 +-
  security/apparmor/procattr.c              |  11 +-
  security/apparmor/resource.c              |  29 +-
  security/apparmor/task.c                  |  14 +-
  35 files changed, 1644 insertions(+), 912 deletions(-)
  create mode 100644 security/apparmor/include/policy_compat.h
  create mode 100644 security/apparmor/policy_compat.c
  

On Wed, Dec 14, 2022 at 10:36 AM John Johansen
<john.johansen@canonical.com> wrote:
>
> John Johansen (45):
>        apparmor: make unpack_array return a trianary value

John, this is unacceptable.

I noticed it due to the conflict, but this really is garbage.

First off, the word is "ternary" (or possibly "tristate").

Secondly, we don't do types like this

    #define tri int

and even if we did, that's a *horrible* name not just for a type, but
for a #define.

Finally, what the heck is "TRI_TRUE/TRI_NONE/TRI_FALSE"? WTF?

It looks like it is used in one single place - the return value for
"unpack_array()" (now renamed "aa_unpack_array()"), and the TRI_FALSE
case is basically an error case for an invalid case.

And TRI_NONE is just a *different* failure case ("name does not exist"
vs "data is invalid").

And then, to make matters worse, ABSOLUTELY NOBODY CARES ABOUT THE
DIFFERENCE. All real users just want to see TRI_TRUE (for "success"),
and anything else is an error anyway.

Yes, yes, there's that one KUNIT test, which wants to actually see
that TRI_FALSE because it's testing that array-out-of-bounds case. It
also - for some unfathomable reason - seems to then want to see some
particular pointer values in that invalid data after the failure,
which seems bogus, but whatever.

In other words, that type is badly done and mis-named to start with,
but then the different ternary values themselves are confusingly
mis-named too in ways that make no sense.

And to cap it all off, NOBODY CARES about those horrid things anyway.

Anyway, I started out doing the mindless conflict resolution, but then
I just couldn't deal with that 'tri' type. There were just too many
things wrong with it for me to accept it, and I felt dirty for just
editing it.

Then I tried out just making it a

     typedef enum { TRI_TRUE/TRI_NONE/TRI_FALSE } ternary_t;

which fixes some of the syntactic issues.

But the whole naming confusion of the values and how NONE-vs-FALSE
wasn't actually a useful distinction anyway made me just axe it
completely.

I'm honestly baffled by why you didn't just make it return the size or
a negative error code, like is the norm. The size is limited to 16
bits anyway, so returning an 'int' with a negative error would have
been very natural.

But just to keep the pattern with some of the other users, and
minimize my surgery, I made it just return 'bool'.

I'm sorry to do all that surgery on it, but I just couldn't stomach
doing anything else.

The resulting merge diff is fairly messy, and to make matters worse I
can't actually *test* any of this. But the code looks more palatable
to me, and I did try to be careful about my surgery.

Apologies if I broke something,

            Linus
  

The pull request you sent on Wed, 14 Dec 2022 10:35:57 -0800:

> git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2022-12-14-merge-breakout

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/93761c93e9da28d8a020777cee2a84133082b477

Thank you!
  

On 12/14/22 13:54, Linus Torvalds wrote:
> On Wed, Dec 14, 2022 at 10:36 AM John Johansen
> <john.johansen@canonical.com> wrote:
>>
>> John Johansen (45):
>>         apparmor: make unpack_array return a trianary value
> 
> John, this is unacceptable.
> 

ack, sorry.

> I noticed it due to the conflict, but this really is garbage.
> 
> First off, the word is "ternary" (or possibly "tristate").
> 
> Secondly, we don't do types like this
> 
>      #define tri int
> 

ack

> and even if we did, that's a *horrible* name not just for a type, but
> for a #define.
> 
> Finally, what the heck is "TRI_TRUE/TRI_NONE/TRI_FALSE"? WTF?
> 
> It looks like it is used in one single place - the return value for
> "unpack_array()" (now renamed "aa_unpack_array()"), and the TRI_FALSE
> case is basically an error case for an invalid case.
> 
> And TRI_NONE is just a *different* failure case ("name does not exist"
> vs "data is invalid").
> 
> And then, to make matters worse, ABSOLUTELY NOBODY CARES ABOUT THE
> DIFFERENCE. All real users just want to see TRI_TRUE (for "success"),
> and anything else is an error anyway.
> 
right, the end goal being not two invalid cases but a case of this
is optional but if not present some default data needs to be tied
in. This can be represented different ways, and using the int as
you suggest below seems like the right way to go.

It also looks like I kicked out the following patch that used this
mess, for further revision and sadly didn't drop this one as well.

> Yes, yes, there's that one KUNIT test, which wants to actually see
> that TRI_FALSE because it's testing that array-out-of-bounds case. It
> also - for some unfathomable reason - seems to then want to see some
> particular pointer values in that invalid data after the failure,
> which seems bogus, but whatever.
> 
> In other words, that type is badly done and mis-named to start with,
> but then the different ternary values themselves are confusingly
> mis-named too in ways that make no sense.
> 
> And to cap it all off, NOBODY CARES about those horrid things anyway.
> 
> Anyway, I started out doing the mindless conflict resolution, but then
> I just couldn't deal with that 'tri' type. There were just too many
> things wrong with it for me to accept it, and I felt dirty for just
> editing it.
> 
> Then I tried out just making it a
> 
>       typedef enum { TRI_TRUE/TRI_NONE/TRI_FALSE } ternary_t;
> 
> which fixes some of the syntactic issues.
> 
> But the whole naming confusion of the values and how NONE-vs-FALSE
> wasn't actually a useful distinction anyway made me just axe it
> completely.
> 

okay

> I'm honestly baffled by why you didn't just make it return the size or
> a negative error code, like is the norm. The size is limited to 16
> bits anyway, so returning an 'int' with a negative error would have
> been very natural.

indeed, tbh I have no clue why. As you say the int type fits right
in with existing kernel code, and doesn't have any range problems.

> 
> But just to keep the pattern with some of the other users, and
> minimize my surgery, I made it just return 'bool'.
> 
okay

> I'm sorry to do all that surgery on it, but I just couldn't stomach
> doing anything else.
> 

understandable

> The resulting merge diff is fairly messy, and to make matters worse I
> can't actually *test* any of this. But the code looks more palatable

I will make sure it gets run through all the testing

> to me, and I did try to be careful about my surgery.
> 
as always, I have no worries about that

> Apologies if I broke something,
> 

none, needed. You did what you needed to do. If needed I will follow-up
with a patch.