[PATCH] audit: Fix check of return value of strnlen_user()

[Paul Moore]

On Thu, Jun 4, 2015 at 3:36 AM, Jan Kara <jack at suse.cz> wrote:
> On Wed 03-06-15 14:56:18, Paul Moore wrote:
>> On Tuesday, June 02, 2015 05:08:29 PM Jan Kara wrote:
>> > strnlen_user() returns 0 when it hits fault, not -1. Fix the test in
>> > audit_log_single_execve_arg(). Luckily this shouldn't ever happen unless
>> > there's a kernel bug so it's mostly a cosmetic fix.
>> >
>> > CC: Paul Moore <pmoore at redhat.com>
>> > Signed-off-by: Jan Kara <jack at suse.cz>
>> > ---
>> >  kernel/auditsc.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 9fb9d1cb83ce..bb947ceeee4d 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -1023,7 +1023,7 @@ static int audit_log_single_execve_arg(struct
>> > audit_context *context, * for strings that are too long, we should not have
>> > created
>> >      * any.
>> >      */
>> > -   if (unlikely((len == -1) || len > MAX_ARG_STRLEN - 1)) {
>> > +   if (unlikely((len == 0) || len > MAX_ARG_STRLEN - 1)) {
>>
>> While we're at it, should we make it just "len > MAX_ARG_STRLEN" as well?
>> Reading the comments in include/uapi/linux/binfmts.h as well as
>> valid_arg_len() that seems to be the correct logic.
>
>   Umm, but audit_log_single_execve_arg() does decrement 1 from
> strnlen_user() result before doing the comparison. So the current test
> seems to match the one in valid_arg_len() exactly...

For reference (taken from fs/exec.c in Linus' tree just now):

  static bool valid_arg_len(struct linux_binprm *bprm, long len)
  {
          return len <= MAX_ARG_STRLEN;
  }

The valid_arg_len() returns true when the length is less than or equal
to MAX_ARG_STRLEN, implying that lengths greater than MAX_ARG_STRLEN
are invalid.  The existing test in audit_log_single_execve_arg()
treats lengths greater than (MAX_ARG_STRLEN-1) as invalid.

These two tests do not look the same to me.

-- 
paul moore
www.paul-moore.com