6.12-stable review patch. If anyone has any objections, please let me know.
------------------
From: Ryan Lee ryan.lee@canonical.com
[ Upstream commit a88db916b8c77552f49f7d9f8744095ea01a268f ]
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 Signed-off-by: Sasha Levin sashal@kernel.org --- security/apparmor/include/match.h | 5 +---- security/apparmor/match.c | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h index 0539abda328d..ae31a8a631fc 100644 --- a/security/apparmor/include/match.h +++ b/security/apparmor/include/match.h @@ -138,15 +138,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 0f791a58d933..12e036f8ce0f 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -624,35 +624,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,