Re: [PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

On Wed, May 28, 2014 at 7:23 PM, Eric Paris <eparis(a)redhat.com&gt; wrote: > On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote: >> Fixes an easy DoS and possible information disclosure. >> >> This does nothing about the broken state of x32 auditing. >> >> Cc: stable(a)vger.kernel.org >> Signed-off-by: Andy Lutomirski <luto(a)amacapital.net&gt; >> --- >> kernel/auditsc.c | 27 ++++++++++++++++++--------- >> 1 file changed, 18 insertions(+), 9 deletions(-) >> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >> index f251a5e..7ccd9db 100644 >> --- a/kernel/auditsc.c >> +++ b/kernel/auditsc.c >> @@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key) >> return AUDIT_BUILD_CONTEXT; >> } >> >> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val) >> +{ >> + int word, bit; >> + >> + if (val > 0xffffffff) >> + return false; > > Why is this necessary? To avoid an integer overflow. Admittedly, this particular overflow won't cause a crash, but it will cause incorrect results.

> >> + >> + word = AUDIT_WORD(val); >> + if (word >= AUDIT_BITMASK_SIZE) >> + return false; > > Since this covers it and it extensible... > >> + >> + bit = AUDIT_BIT(val); >> + >> + return rule->mask[word] & bit; > > Returning an int as a bool creates worse code than just returning the > int. (although in this case if the compiler chooses to inline it might > be smart enough not to actually convert this int to a bool and make > worse assembly...) I'd suggest the function here return an int. bools > usually make the assembly worse... I'm ambivalent. The right assembly would use flags on x86, not an int or a bool, and I don't know what the compiler will do.