* [PATCH] audit: Fix check of return value of strnlen_user()
@ 2015-06-02 15:08 Jan Kara
2015-06-03 18:56 ` Paul Moore
2015-06-11 19:58 ` Paul Moore
0 siblings, 2 replies; 7+ messages in thread
From: Jan Kara @ 2015-06-02 15:08 UTC (permalink / raw)
To: linux-audit; +Cc: Jan Kara
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@redhat.com>
Signed-off-by: Jan Kara <jack@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)) {
WARN_ON(1);
send_sig(SIGKILL, current, 0);
return -1;
--
2.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] audit: Fix check of return value of strnlen_user()
2015-06-02 15:08 [PATCH] audit: Fix check of return value of strnlen_user() Jan Kara
@ 2015-06-03 18:56 ` Paul Moore
2015-06-04 7:36 ` Jan Kara
2015-06-11 19:58 ` Paul Moore
1 sibling, 1 reply; 7+ messages in thread
From: Paul Moore @ 2015-06-03 18:56 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-audit
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@redhat.com>
> Signed-off-by: Jan Kara <jack@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.
> WARN_ON(1);
> send_sig(SIGKILL, current, 0);
> return -1;
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] audit: Fix check of return value of strnlen_user()
2015-06-03 18:56 ` Paul Moore
@ 2015-06-04 7:36 ` Jan Kara
2015-06-04 13:18 ` Paul Moore
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2015-06-04 7:36 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, Jan Kara
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@redhat.com>
> > Signed-off-by: Jan Kara <jack@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...
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] audit: Fix check of return value of strnlen_user()
2015-06-04 7:36 ` Jan Kara
@ 2015-06-04 13:18 ` Paul Moore
2015-06-04 21:32 ` Jan Kara
0 siblings, 1 reply; 7+ messages in thread
From: Paul Moore @ 2015-06-04 13:18 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-audit
On Thu, Jun 4, 2015 at 3:36 AM, Jan Kara <jack@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@redhat.com>
>> > Signed-off-by: Jan Kara <jack@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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] audit: Fix check of return value of strnlen_user()
2015-06-04 13:18 ` Paul Moore
@ 2015-06-04 21:32 ` Jan Kara
2015-06-04 21:48 ` Paul Moore
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2015-06-04 21:32 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, Jan Kara
On Thu 04-06-15 09:18:49, Paul Moore wrote:
> On Thu, Jun 4, 2015 at 3:36 AM, Jan Kara <jack@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@redhat.com>
> >> > Signed-off-by: Jan Kara <jack@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.
But the 'len' passed to valid_arg_len() is the return value of
strnlen_user() and that one returns lenght *including* the terminating
'\0'. So in fact valid_arg_len() tests whether the argument length is <=
(MAX_ARG_STRLEN-1). Hmm?
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] audit: Fix check of return value of strnlen_user()
2015-06-04 21:32 ` Jan Kara
@ 2015-06-04 21:48 ` Paul Moore
0 siblings, 0 replies; 7+ messages in thread
From: Paul Moore @ 2015-06-04 21:48 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-audit
On Thu, Jun 4, 2015 at 5:32 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 04-06-15 09:18:49, Paul Moore wrote:
>> On Thu, Jun 4, 2015 at 3:36 AM, Jan Kara <jack@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@redhat.com>
>> >> > Signed-off-by: Jan Kara <jack@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.
> But the 'len' passed to valid_arg_len() is the return value of
> strnlen_user() and that one returns lenght *including* the terminating
> '\0'. So in fact valid_arg_len() tests whether the argument length is <=
> (MAX_ARG_STRLEN-1). Hmm?
I must have spaced on the fact that audit subtracts the NULL from the
output of strnlen_user() when it calls it earlier in the function.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] audit: Fix check of return value of strnlen_user()
2015-06-02 15:08 [PATCH] audit: Fix check of return value of strnlen_user() Jan Kara
2015-06-03 18:56 ` Paul Moore
@ 2015-06-11 19:58 ` Paul Moore
1 sibling, 0 replies; 7+ messages in thread
From: Paul Moore @ 2015-06-11 19:58 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-audit
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@redhat.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> kernel/auditsc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged to audit#next, thanks for your patience.
> 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)) {
> WARN_ON(1);
> send_sig(SIGKILL, current, 0);
> return -1;
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-06-11 19:58 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-02 15:08 [PATCH] audit: Fix check of return value of strnlen_user() Jan Kara
2015-06-03 18:56 ` Paul Moore
2015-06-04 7:36 ` Jan Kara
2015-06-04 13:18 ` Paul Moore
2015-06-04 21:32 ` Jan Kara
2015-06-04 21:48 ` Paul Moore
2015-06-11 19:58 ` Paul Moore
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).