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

[Dan Carpenter]

[ 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:

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