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
