On Tue, Aug 30, 2016 at 3:09 PM, Josh Poimboeuf <jpoim...@redhat.com> wrote:
> On Tue, Aug 30, 2016 at 02:15:58PM -0400, Kees Cook wrote:
>> static inline __must_check unsigned long __copy_from_user(void *to,
>> const void __user *from, unsigned long n)
>> {
>> int dest_size = __compiletime_object_size(to);
>>
>> might_fault();
>> /* KASan seems to want pre-check arguments, so run it first. */
>> kasan_check_write(to, n);
>>
>> if (likely(dest_size != -1)) {
>> /* Destination object size is known at compile time. */
>> if (n > dest_size) {
>> /* Copy size is too large for destination object. */
>> if (__builtin_constant_p(n)) {
>> /* Copy size is known at compile time: abort the build. */
>> copy_user_compile_time_overflow(dest_size, n);
>> } else {
>> /* Copy size only known at runtime, abort copy with BUG. */
>> __bad_user_copy();
>> }
>> } else {
>> /* Copy size within size of destination object, perform copy. */
>> n = __arch_copy_from_user(to, from, n);
>> }
>> } else {
>> /* Destination object size needs runtime checking. */
>> check_runtime_object_size(to, from, n);
>> /* If we got here, runtime checks passed, perform copy. */
>> n = __arch_copy_from_user(to, from, n);
>> }
>> return n;
>> }
>>
>> static inline __must_check unsigned long copy_from_user(void *to,
>> const void __user * from, unsigned long n)
>> {
>> if (access_ok(VERIFY_READ, from, n)) {
>> n = __copy_from_user(to, from, n);
>> } else
>> memset(to, 0, n); /* This is needed to avoid memory
>> content leaks. */
>> return n;
>> }
>>
>> Some notes, here: the __bad_user_copy() should be a BUG, not a WARN
>> since we've landed on a provably bad situation.
>
> Looks good to me. One nit: I think the "likely" check for "dest_size !=
> -1" isn't needed. dest_size is known at compile-time, so gcc should be
> able to optimize it accordingly.
Yeah, good point.
>> check_object_size() should probably be renamed
>> "check_runtime_obj_size" or something to clarify its purpose, since
>> it's intended to be called only when we have to go off and examine
>> runtime object metadata to figure out how to correctly perform bounds
>> checking.
>
> Personally I find having "size" in the name to be misleading, since the
> function actually looks at much more than just size. Especially
> considering the fact that we already have the other static and runtime
> checks which do only check the size.
>
> I also don't really care for "runtime", since most functions are indeed
> called at runtime. If anything I'd prefer the reverse, where any
> built-in compile-time "functions" are specially named or annotated.
>
> My vote would be something like check_usercopy_object().
Sounds good to me. :)
-Kees
--
Kees Cook
Nexus Security
