On Jun 1, 2015, at 3:31 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> On Tue, May 26, 2015 at 01:48:47PM -0400, Chuck Lever wrote:
>> Ensure a proper memory access check is done by read_reset_stat(),
>> then fix the following compiler warning.
>>
>> In file included from linux-2.6/include/net/checksum.h:25,
>> from linux-2.6/include/linux/skbuff.h:31,
>> from linux-2.6/include/linux/icmpv6.h:4,
>> from linux-2.6/include/linux/ipv6.h:64,
>> from linux-2.6/include/net/ipv6.h:16,
>> from linux-2.6/include/linux/sunrpc/clnt.h:27,
>> from linux-2.6/net/sunrpc/xprtrdma/svc_rdma.c:47:
>> In function ‘copy_to_user’,
>> inlined from ‘read_reset_stat’ at
>> linux-2.6/net/sunrpc/xprtrdma/svc_rdma.c:113:
>> linux-2.6/arch/x86/include/asm/uaccess.h:735: warning:
>> call to ‘__copy_to_user_overflow’ declared with attribute warning:
>> copy_to_user() buffer size is not provably correct
>
> How do you get that warning? I can't hit it even with
> CONFIG_USER_STRICT_USER_COPY_CHECKS set. Based on comments in arch/x86
> I would have thought this would only trigger when len was a constant.
I only seem to see this warning when building and testing on EL6, gcc
version 4.4.7 20120313 (Red Hat 4.4.7-11) (GCC).
include/linux/compiler-gcc4.h has this:
16 #if GCC_VERSION >= 40100 && GCC_VERSION < 40600
17 # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
18 #endif
and include/linux/compiler.h has this:
384 #ifndef __compiletime_object_size
385 # define __compiletime_object_size(obj) -1
386 #endif
Now, arch/x86/include/asm/uaccess.h spells copy_to_user() this way:
722 static inline unsigned long __must_check
723 copy_to_user(void __user *to, const void *from, unsigned long n)
724 {
725 int sz = __compiletime_object_size(from);
726
727 might_fault();
728
729 /* See the comment in copy_from_user() above. */
730 if (likely(sz < 0 || sz >= n))
731 n = _copy_to_user(to, from, n);
732 else if(__builtin_constant_p(n))
733 copy_to_user_overflow();
734 else
735 __copy_to_user_overflow(sz, n);
736
737 return n;
738 }
If __compiletime_object_size isn’t defined for your compiler version, then
int sz is set to -1, and _copy_to_user() is invoked directly. Otherwise
if n is a variable, the warning pops.
This might be a false positive. The “from” parameter is always 32 bytes in
read_reset_stat().
I think adding an access_ok() call here is reasonable, though?
> --b.
>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> ---
>>
>> net/sunrpc/xprtrdma/svc_rdma.c | 8 ++++++--
>> 1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c
>> index c1b6270..8eedb60 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma.c
>> @@ -98,7 +98,11 @@ static int read_reset_stat(struct ctl_table *table, int write,
>> else {
>> char str_buf[32];
>> char *data;
>> - int len = snprintf(str_buf, 32, "%d\n", atomic_read(stat));
>> + int len;
>> +
>> + if (!access_ok(VERIFY_WRITE, buffer, *lenp))
>> + return -EFAULT;
>> + len = snprintf(str_buf, 32, "%d\n", atomic_read(stat));
>> if (len >= 32)
>> return -EFAULT;
>> len = strlen(str_buf);
>> @@ -110,7 +114,7 @@ static int read_reset_stat(struct ctl_table *table, int write,
>> len -= *ppos;
>> if (len > *lenp)
>> len = *lenp;
>> - if (len && copy_to_user(buffer, str_buf, len))
>> + if (len && __copy_to_user(buffer, str_buf, len))
>> return -EFAULT;
>> *lenp = len;
>> *ppos += len;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
