[PATCH] - Fix wrong check in aux_syscalls.stp:_struct_sockaddr_u_impl (related to PR16718)

[PATCH] - Fix wrong check in aux_syscalls.stp:_struct_sockaddr_u_impl (related to PR16718)

[Robin Hack]

[-- Attachment #1: Type: text/plain, Size: 113 bytes --]

Hi.

This small patch according to PR16718 fixes wrong check of STAP_ARG_len argument.

Have nice day
Robin Hack

[-- Attachment #2: aux_syscalls-arg-check.patch --]
[-- Type: text/plain, Size: 848 bytes --]

commit 12d477cbc8e472a3c7820c3b276c5c535b0f2791
Author: Robin Hack <rhack@redhat.com>
Date:   Tue Mar 18 13:11:41 2014 +0100

    aux_syscalls: Fix STAP_ARG_len wrong check.

diff --git a/tapset/linux/aux_syscalls.stp b/tapset/linux/aux_syscalls.stp
index c99ef976d907391d328bf64ef0f6a574b5e2b8a5..107cde5366df45e784985a15e7071e0bd3e47de9 100644
--- a/tapset/linux/aux_syscalls.stp
+++ b/tapset/linux/aux_syscalls.stp
@@ -389,7 +389,7 @@ function _struct_sockaddr_u_impl:string(uaddr:long, len:long, what:long)
     sa_dispatch what = (sa_dispatch)STAP_ARG_what;
 
     char *ptr = (char *)(unsigned long)STAP_ARG_uaddr;
-    size_t len = STAP_ARG_len < 128 ? STAP_ARG_len: 128;
+    size_t len = max(128, STAP_ARG_len);
     struct sockaddr *sa = (struct sockaddr *)CONTEXT->buf;
 
     char *stap_retvalue = (char *)(unsigned long)STAP_RETVALUE;

Re: [PATCH] - Fix wrong check in aux_syscalls.stp:_struct_sockaddr_u_impl (related to PR16718)

[David Smith]

On 03/18/2014 07:15 AM, Robin Hack wrote:
> Hi.
> 
> This small patch according to PR16718 fixes wrong check of STAP_ARG_len argument.

I had thought of doing something like this, but figured the new
_stp_copy_from_user() check would work here. I didn't see any harm in
your patch though, so I checked it in.

I then thought about it some more. Your patch did this:

-    size_t len = STAP_ARG_len < 128 ? STAP_ARG_len: 128;
+    size_t len = max(128, STAP_ARG_len);

Depending on when the conversion from signed to unsigned happens, -1 is
still less than 128. So, in commit 0a01aa9 I tightened that up to:

    size_t len = clamp((size_t)STAP_ARG_len, (size_t)0, (size_t)128);

This way we're sure that the value is between 0 and 128. I also went
through the rest of the calls to _stp_copy_from_user(), making sure the
input length was reasonable.

Thanks for the patch.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [PATCH] - Fix wrong check in aux_syscalls.stp:_struct_sockaddr_u_impl (related to PR16718)

[Josh Stone]

On 03/18/2014 07:59 AM, David Smith wrote:
>     size_t len = clamp((size_t)STAP_ARG_len, (size_t)0, (size_t)128);

Consider using clamp_t to force a particular type.

Re: [PATCH] - Fix wrong check in aux_syscalls.stp:_struct_sockaddr_u_impl (related to PR16718)

[David Smith]

On 03/18/2014 11:58 AM, Josh Stone wrote:
> On 03/18/2014 07:59 AM, David Smith wrote:
>>     size_t len = clamp((size_t)STAP_ARG_len, (size_t)0, (size_t)128);
> 
> Consider using clamp_t to force a particular type.

Ah, nice. Commit 4cddafd switches to using clamp_t.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [PATCH] - Fix wrong check in aux_syscalls.stp:_struct_sockaddr_u_impl (related to PR16718)

[Robin Hack]

On Tue, Mar 18, 2014 at 02:59:33PM -0500, David Smith wrote:
> On 03/18/2014 11:58 AM, Josh Stone wrote:
> > On 03/18/2014 07:59 AM, David Smith wrote:
> >>     size_t len = clamp((size_t)STAP_ARG_len, (size_t)0, (size_t)128);
> > 
> > Consider using clamp_t to force a particular type.
> 
> Ah, nice. Commit 4cddafd switches to using clamp_t.
I dug in the kernel history and I found that clamp_t is supported from
kernel version 2.6.26. Is this ok?
> 
> -- 
> David Smith
> dsmith@redhat.com
> Red Hat
> http://www.redhat.com
> 256.217.0141 (direct)
> 256.837.0057 (fax)

Re: [PATCH] - Fix wrong check in aux_syscalls.stp:_struct_sockaddr_u_impl (related to PR16718)

[David Smith]

On 03/19/2014 03:25 AM, Robin Hack wrote:
> On Tue, Mar 18, 2014 at 02:59:33PM -0500, David Smith wrote:
>> On 03/18/2014 11:58 AM, Josh Stone wrote:
>>> On 03/18/2014 07:59 AM, David Smith wrote:
>>>>     size_t len = clamp((size_t)STAP_ARG_len, (size_t)0, (size_t)128);
>>>
>>> Consider using clamp_t to force a particular type.
>>
>> Ah, nice. Commit 4cddafd switches to using clamp_t.
> I dug in the kernel history and I found that clamp_t is supported from
> kernel version 2.6.26. Is this ok?

We have our own copy of clamp_t in runtine/linux/runtime.h, so we should
be OK.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)