[bug report] apparmor: add support loading per permission tagging

[bug report] apparmor: add support loading per permission tagging

[Dan Carpenter]

Hello John Johansen,

Commit 3d28e2397af7 ("apparmor: add support loading per permission
tagging") from Apr 1, 2025 (linux-next), leads to the following
Smatch static checker warning:

	security/apparmor/policy_unpack.c:883 unpack_tags()
	warn: missing error code 'error'

security/apparmor/policy_unpack.c
    852 static int unpack_tags(struct aa_ext *e, struct aa_tags_struct *tags,
    853         const char **info)
    854 {
    855         int error = -EPROTO;
    856         void *pos = e->pos;
    857 
    858         AA_BUG(!tags);
    859         /* policy tags are optional */
    860         if (aa_unpack_nameX(e, AA_STRUCT, "tags")) {
    861                 u32 version;
    862 
    863                 if (!aa_unpack_u32(e, &version, "version") || version != 1) {
    864                         *info = "invalid tags version";
    865                         goto fail_reset;
    866                 }
    867                 error = unpack_strs_table(e, "strs", true, &tags->strs);
    868                 if (error) {
    869                         *info = "failed to unpack profile tag.strs";
    870                         goto fail;
    871                 }
    872                 error = unpack_tag_headers(e, tags);
    873                 if (error) {
    874                         *info = "failed to unpack profile tag.headers";
    875                         goto fail;
    876                 }
    877                 error = unpack_tagsets(e, tags);
    878                 if (error) {
    879                         *info = "failed to unpack profile tag.sets";
    880                         goto fail;
    881                 }
    882                 if (!aa_unpack_nameX(e, AA_STRUCTEND, NULL))
--> 883                         goto fail;

set the error code here

    884 
    885                 if (!verify_tags(tags, info))
    886                         goto fail;

and here

    887         }
    888 
    889         return 0;
    890 
    891 fail:
    892         aa_destroy_tags(tags);
    893 fail_reset:
    894         e->pos = pos;
    895         return error;
    896 }

This email is a free service from the Smatch-CI project [smatch.sf.net].

regards,
dan carpenter

Re: Support needed to continue Smatch work

[Dan Carpenter]

I need to post an update on the current situation with Smatch.

First of all, I want to start by thanking Brad Spengler from grsecurity
who reached out to me on this, offered some funding, and has been
trying to push the Smatch work forward.  It really means a lot to me.

Unfortunately, we haven't been able to raise enough support to continue
my Smatch work.  I have still been filtering zero day bot warnings and
I am a bit worried that people have the impression that I'm reviewing
static checker warnings when I am not.

The situation isn't great.  The zero day bot can't do cross function
analsysis and it only looks at checks with a low false positive rate.
We're missing out on a bunch of bugs.  I'm going to add some of the
those missed warnings to this thread so people have a better picture of
what we're missing.  There are some buffer overflows in there.  A bunch
of off by one bugs.  A missing error code in fork().  And random other
minor things as well.

https://lore.kernel.org/all/caa37f28-a2e8-4e0a-a9ce-a365ce805e4b@stanley.mountain/

I am still trying to figure out a way to restart Smatch checking.  The
funding model would be that several companies would support this project
by paying a proportion of my salary.  Part of that goes to reporting
bugs like the ones above and part of that goes to developing Smatch and
writing new checks.  Please, contact
Bill Fletcher <bill.fletcher@linaro.org> if you would like to support
this work.

regards,
dan carpenter

[bug report] apparmor: add support loading per permission tagging

[Dan Carpenter]

[ Smatch checking is paused while we raise funding. #SadFace
  https://lore.kernel.org/all/aTaiGSbWZ9DJaGo7@stanley.mountain/ -dan ]

Hello John Johansen,

Commit 3d28e2397af7 ("apparmor: add support loading per permission
tagging") from Apr 1, 2025 (linux-next), leads to the following
Smatch static checker warning:

	security/apparmor/policy_unpack.c:966 unpack_pdb()
	warn: unsigned 'unpack_tags(e, &pdb->tags, info)' is never less than zero.

security/apparmor/policy_unpack.c
    951 static int unpack_pdb(struct aa_ext *e, struct aa_policydb **policy,
    952                       bool required_dfa, bool required_trans,
    953                       const char **info)
    954 {
    955         struct aa_policydb *pdb;
    956         void *pos = e->pos;
    957         int i, flags, error = -EPROTO;
    958         ssize_t size;
    959         u32 version = 0;
    960 
    961         pdb = aa_alloc_pdb(GFP_KERNEL);
    962         if (!pdb)
    963                 return -ENOMEM;
    964 
    965         AA_DEBUG(DEBUG_UNPACK, "unpacking tags");
--> 966         if (unpack_tags(e, &pdb->tags, info) < 0)
                    ^^^^^^^^^^^
Signedness bug.  unpack_tags() is unsigned.

    967                 goto fail;
    968         AA_DEBUG(DEBUG_UNPACK, "done unpacking tags");
    969 
    970         size = unpack_perms_table(e, &pdb->perms);
    971         if (size < 0) {
    972                 error = size;
    973                 pdb->perms = NULL;

regards,
dan carpenter