lkml.org 
[lkml]   [2016]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
    On Tue, Aug 30, 2016 at 3:09 PM, Josh Poimboeuf <jpoimboe@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

    \
     
     \ /
      Last update: 2016-09-17 09:58    [W:4.768 / U:0.972 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site