[apparmor] [bug report] apparmor: add additional flags to extended permission.

[John Johansen]

On 1/21/25 03:17, Dan Carpenter wrote:
> [ I don't know why this static checker warning is showing up only now
>    two years later...  -dan ]
> 
> Hello John Johansen,
> 
> Commit 2e12c5f06017 ("apparmor: add additional flags to extended
> permission.") from Jul 23, 2023 (linux-next), leads to the following
> Smatch static checker warning:
> 

because while it is an old commit it sat in other branches and dev
trees, while the stuff that depends on it was revised, and that stuff is
just recently starting to be merged in.

Thanks for the report, I see if I can't get this fixed today

> security/apparmor/policy_unpack.c:780 unpack_pdb()
> error: we previously assumed 'pdb->dfa' could be null (see line 753)
> 
> security/apparmor/policy_unpack.c
>      712 static int unpack_pdb(struct aa_ext *e, struct aa_policydb **policy,
>      713                       bool required_dfa, bool required_trans,
>      714                       const char **info)
>      715 {
>      716         struct aa_policydb *pdb;
>      717         void *pos = e->pos;
>      718         int i, flags, error = -EPROTO;
>      719         ssize_t size;
>      720         u32 version = 0;
>      721
>      722         pdb = aa_alloc_pdb(GFP_KERNEL);
>      723         if (!pdb)
>      724                 return -ENOMEM;
>      725
>      726         size = unpack_perms_table(e, &pdb->perms);
>      727         if (size < 0) {
>      728                 error = size;
>      729                 pdb->perms = NULL;
>      730                 *info = "failed to unpack - perms";
>      731                 goto fail;
>      732         }
>      733         pdb->size = size;
>      734
>      735         if (pdb->perms) {
>      736                 /* perms table present accept is index */
>      737                 flags = TO_ACCEPT1_FLAG(YYTD_DATA32);
>      738                 if (aa_unpack_u32(e, &version, "permsv") && version > 2)
>      739                         /* accept2 used for dfa flags */
>      740                         flags |= TO_ACCEPT2_FLAG(YYTD_DATA32);
>      741         } else {
>      742                 /* packed perms in accept1 and accept2 */
>      743                 flags = TO_ACCEPT1_FLAG(YYTD_DATA32) |
>      744                         TO_ACCEPT2_FLAG(YYTD_DATA32);
>      745         }
>      746
>      747         pdb->dfa = unpack_dfa(e, flags);
>      748         if (IS_ERR(pdb->dfa)) {
>      749                 error = PTR_ERR(pdb->dfa);
>      750                 pdb->dfa = NULL;
>      751                 *info = "failed to unpack - dfa";
>      752                 goto fail;
>      753         } else if (!pdb->dfa) {
>      754                 if (required_dfa) {
>      755                         *info = "missing required dfa";
>      756                         goto fail;
>      757                 }
> 
> Assume required_dfa is false.
> 
> 
>      758         } else {
>      759                 /*
>      760                  * only unpack the following if a dfa is present
>      761                  *
>      762                  * sadly start was given different names for file and policydb
>      763                  * but since it is optional we can try both
>      764                  */
>      765                 if (!aa_unpack_u32(e, &pdb->start[0], "start"))
>      766                         /* default start state */
>      767                         pdb->start[0] = DFA_START;
>      768                 if (!aa_unpack_u32(e, &pdb->start[AA_CLASS_FILE], "dfa_start")) {
>      769                         /* default start state for xmatch and file dfa */
>      770                         pdb->start[AA_CLASS_FILE] = DFA_START;
>      771                 }        /* setup class index */
>      772                 for (i = AA_CLASS_FILE + 1; i <= AA_CLASS_LAST; i++) {
>      773                         pdb->start[i] = aa_dfa_next(pdb->dfa, pdb->start[0],
>      774                                                     i);
>      775                 }
>      776         }
>      777
>      778         if (pdb->perms && version <= 2) {
>      779                 /* add dfa flags table missing in v2 */
> --> 780                 u32 noents = pdb->dfa->tables[YYTD_ID_ACCEPT]->td_lolen;
>                                       ^^^^^^^^^^
> Potential NULL pointer dereference
> 
>      781                 u16 tdflags = pdb->dfa->tables[YYTD_ID_ACCEPT]->td_flags;
>      782                 size_t tsize = table_size(noents, tdflags);
>      783
>      784                 pdb->dfa->tables[YYTD_ID_ACCEPT2] = kvzalloc(tsize, GFP_KERNEL);
>      785                 if (!pdb->dfa->tables[YYTD_ID_ACCEPT2]) {
>      786                         *info = "failed to alloc dfa flags table";
>      787                         goto out;
>      788                 }
>      789                 pdb->dfa->tables[YYTD_ID_ACCEPT2]->td_lolen = noents;
>      790                 pdb->dfa->tables[YYTD_ID_ACCEPT2]->td_flags = tdflags;
>      791         }
>      792         /*
>      793          * Unfortunately due to a bug in earlier userspaces, a
>      794          * transition table may be present even when the dfa is
>      795          * not. For compatibility reasons unpack and discard.
>      796          */
>      797         if (!unpack_trans_table(e, &pdb->trans) && required_trans) {
>      798                 *info = "failed to unpack profile transition table";
>      799                 goto fail;
>      800         }
>      801
>      802         if (!pdb->dfa && pdb->trans.table)
>      803                 aa_free_str_table(&pdb->trans);
>      804
>      805         /* TODO: move compat mapping here, requires dfa merging first */
>      806         /* TODO: move verify here, it has to be done after compat mappings */
>      807 out:
>      808         *policy = pdb;
>      809         return 0;
>      810
>      811 fail:
>      812         aa_put_pdb(pdb);
>      813         e->pos = pos;
>      814         return error;
>      815 }
> 
> regards,
> dan carpenter