apparmor: fix loop detection used in conflicting attachment resolution

Conflicting attachment resolution is based on the number of states
traversed to reach an accepting state in the attachment DFA, accounting
for DFA loops traversed during the matching process. However, the loop
counting logic had multiple bugs:

 - The inc_wb_pos macro increments both position and length, but length
   is supposed to saturate upon hitting buffer capacity, instead of
   wrapping around.
 - If no revisited state is found when traversing the history, is_loop
   would still return true, as if there was a loop found the length of
   the history buffer, instead of returning false and signalling that
   no loop was found. As a result, the adjustment step of
   aa_dfa_leftmatch would sometimes produce negative counts with loop-
   free DFAs that traversed enough states.
 - The iteration in the is_loop for loop is supposed to stop before
   i = wb->len, so the conditional should be < instead of <=.

This patch fixes the above bugs as well as the following nits:
 - The count and size fields in struct match_workbuf were not used,
   so they can be removed.
 - The history buffer in match_workbuf semantically stores aa_state_t
   and not unsigned ints, even if aa_state_t is currently unsigned int.
 - The local variables in is_loop are counters, and thus should be
   unsigned ints instead of aa_state_t's.

Fixes: 21f606610502 ("apparmor: improve overlapping domain attachment resolution")
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Co-developed-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>

diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
index 21e049b408246ae39e5c1d99a020bce60d8a9ec8..1fbe82f5021b16886025233672f07dd25784c12e 100644 (file)
--- a/security/apparmor/include/match.h
+++ b/security/apparmor/include/match.h
@@ -140,15 +140,12 @@ void aa_dfa_free_kref(struct kref *kref);
 /* This needs to be a power of 2 */
 #define WB_HISTORY_SIZE 32
 struct match_workbuf {
-       unsigned int count;
        unsigned int pos;
        unsigned int len;
-       unsigned int size;      /* power of 2, same as history size */
-       unsigned int history[WB_HISTORY_SIZE];
+       aa_state_t history[WB_HISTORY_SIZE];
 };
 #define DEFINE_MATCH_WB(N)             \
 struct match_workbuf N = {             \
-       .count = 0,                     \
        .pos = 0,                       \
        .len = 0,                       \
 }

diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index 1ceabde550f2c7520daca499c6dac5ba9e793df7..c5a91600842a161843945699ce27661da0457e48 100644 (file)
--- a/security/apparmor/match.c
+++ b/security/apparmor/match.c
@@ -679,35 +679,35 @@ aa_state_t aa_dfa_matchn_until(struct aa_dfa *dfa, aa_state_t start,
        return state;
 }
 
-#define inc_wb_pos(wb)                                         \
-do {                                                           \
+#define inc_wb_pos(wb)                                                 \
+do {                                                                   \
        BUILD_BUG_ON_NOT_POWER_OF_2(WB_HISTORY_SIZE);                   \
        wb->pos = (wb->pos + 1) & (WB_HISTORY_SIZE - 1);                \
-       wb->len = (wb->len + 1) & (WB_HISTORY_SIZE - 1);                \
+       wb->len = (wb->len + 1) > WB_HISTORY_SIZE ? WB_HISTORY_SIZE :   \
+                               wb->len + 1;                            \
 } while (0)
 
 /* For DFAs that don't support extended tagging of states */
+/* adjust is only set if is_loop returns true */
 static bool is_loop(struct match_workbuf *wb, aa_state_t state,
                    unsigned int *adjust)
 {
-       aa_state_t pos = wb->pos;
-       aa_state_t i;
+       int pos = wb->pos;
+       int i;
 
        if (wb->history[pos] < state)
                return false;
 
-       for (i = 0; i <= wb->len; i++) {
+       for (i = 0; i < wb->len; i++) {
                if (wb->history[pos] == state) {
                        *adjust = i;
                        return true;
                }
-               if (pos == 0)
-                       pos = WB_HISTORY_SIZE;
-               pos--;
+               /* -1 wraps to WB_HISTORY_SIZE - 1 */
+               pos = (pos - 1) & (WB_HISTORY_SIZE - 1);
        }
 
-       *adjust = i;
-       return true;
+       return false;
 }
 
 static aa_state_t leftmatch_fb(struct aa_dfa *dfa, aa_state_t start,