Re: [PATCH 5.10 430/491] apparmor: validate DFA start states are in bounds in unpack_pdb

Ben Hutchings
2026-04-26T13:44:14Z

[-- Attachment #1: Type: text/plain, Size: 4058 bytes --]

On Tue, 2026-04-21 at 01:42 -0700, John Johansen wrote:
> On 4/19/26 03:26, Ben Hutchings wrote:
> > On Mon, 2026-04-13 at 18:01 +0200, Greg Kroah-Hartman wrote:
> > > 5.10-stable review patch. If anyone has any objections, please let me know.
> > >
> > > ------------------
> > >
> > > From: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com>
> > >
> > > commit 9063d7e2615f4a7ab321de6b520e23d370e58816 upstream.
> > >
> > > Backport for conflicts caused by
> > > ad596ea74e74 ("apparmor: group dfa policydb unpacking")
> > > - rearrange and consolidated the unpack.
> > >
> > > b11e51dd7094 ("apparmor: test: make static symbols visible during kunit testing")
> > > - rename function and make it visible to kunit tests
> > >
> > > Start states are read from untrusted data and used as indexes into the
> > > DFA state tables. The aa_dfa_next() function call in unpack_pdb() will
> > > access dfa->tables[YYTD_ID_BASE][start], and if the start state exceeds
> > > the number of states in the DFA, this results in an out-of-bound read.
> > >
> > > ==================================================================
> > > BUG: KASAN: slab-out-of-bounds in aa_dfa_next+0x2a1/0x360
> > > Read of size 4 at addr ffff88811956fb90 by task su/1097
> > > ...
> > >
> > > Reject policies with out-of-bounds start states during unpacking
> > > to prevent the issue.
> > >
> > > Fixes: ad5ff3db53c6 ("AppArmor: Add ability to load extended policy")
> > > Reported-by: Qualys Security Advisory <qsa@qualys.com>
> > > Tested-by: Salvatore Bonaccorso <carnil@debian.org>
> > > Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
> > > Reviewed-by: Cengiz Can <cengiz.can@canonical.com>
> > > Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com>
> > > Signed-off-by: John Johansen <john.johansen@canonical.com>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > > security/apparmor/policy_unpack.c | 21 +++++++++++++++++++--
> > > 1 file changed, 19 insertions(+), 2 deletions(-)
> > >
> > > --- a/security/apparmor/policy_unpack.c
> > > +++ b/security/apparmor/policy_unpack.c
> > > @@ -841,9 +841,18 @@ static struct aa_profile *unpack_profile
> > > error = -EPROTO;
> > > goto fail;
> > > }
> > > - if (!unpack_u32(e, &profile->policy.start[0], "start"))
> > > + if (!unpack_u32(e, &profile->policy.start[0], "start")) {
> > > /* default start state */
> > > profile->policy.start[0] = DFA_START;
> > > + } else {
> > > + size_t state_count = profile->policy.dfa->tables[YYTD_ID_BASE]->td_lolen;
> > > +
> > > + if (profile->policy.start[0] >= state_count) {
> > > + info = "invalid dfa start state";
> > > + goto fail;
> > > + }
> > > + }
> > [...]
> >
> > Isn't this range check needed even if we use the default start state?
> > unpack_table() only checks that td_tolen > 0, so we could end up with
> > profile->policy.start[0] = DFA_START == 1 and
> > profile->policy.dfa->tables[YYTD_ID_BASE]->td_lolen == 1.
> >
> > (This is specific to the backport as the upstream version didn't put
> > this check in an else-block.)
> >
>
> Hey Ben,
> I specifically chose not to make that alteration to the patches sent to
> stable after reviewing the submission rules. That would be a non-backport
> related change from what landed upstream and we were referencing as the
> upstream patch.

But the upstream patch didn't have this problem! The range check was
not conditional so it would catch this case.

> Instead I am sending Linus a 2nd patch that addresses
> the issue by ensure the loaded dfa has at least two states. It will has
> a fixes tag and will get pulled back.

I think you are talking about adding a check in unpack_table(). I am
talking about the later check in unpack_profile() (or in unpack_pdb()
upstream).

Ben.

--
Ben Hutchings
Nothing is ever a complete failure;
it can always serve as a bad example.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]