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> 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>
> ---
> 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.
--Andy