• Daniel Mentz's avatar
    media: v4l2-compat-ioctl32.c: refactor compat ioctl32 logic · a1dfb4c4
    Daniel Mentz authored
    The 32-bit compat v4l2 ioctl handling is implemented based on its 64-bit
    equivalent. It converts 32-bit data structures into its 64-bit
    equivalents and needs to provide the data to the 64-bit ioctl in user
    space memory which is commonly allocated using
    compat_alloc_user_space().
    
    However, due to how that function is implemented, it can only be called
    a single time for every syscall invocation.
    
    Supposedly to avoid this limitation, the existing code uses a mix of
    memory from the kernel stack and memory allocated through
    compat_alloc_user_space().
    
    Under normal circumstances, this would not work, because the 64-bit
    ioctl expects all pointers to point to user space memory. As a
    workaround, set_fs(KERNEL_DS) is called to temporarily disable this
    extra safety check and allow kernel pointers. However, this might
    introduce a security vulnerability: The result of the 32-bit to 64-bit
    conversion is writeable by user space because the output buffer has been
    allocated via compat_alloc_user_space(). A malicious user space process
    could then manipulate pointers inside this output buffer, and due to the
    previous set_fs(KERNEL_DS) call, functions like get_user() or put_user()
    no longer prevent kernel memory access.
    
    The new approach is to pre-calculate the total amount of user space
    memory that is needed, allocate it using compat_alloc_user_space() and
    then divide up the allocated memory to accommodate all data structures
    that need to be converted.
    
    An alternative approach would have been to retain the union type karg
    that they allocated on the kernel stack in do_video_ioctl(), copy all
    data from user space into karg and then back to user space. However, we
    decided against this approach because it does not align with other
    compat syscall implementations. Instead, we tried to replicate the
    get_user/put_user pairs as found in other places in the kernel:
    
        if (get_user(clipcount, &up->clipcount) ||
            put_user(clipcount, &kp->clipcount)) return -EFAULT;
    
    Notes from hans.verkuil@cisco.com:
    
    This patch was taken from:
        https://github.com/LineageOS/android_kernel_samsung_apq8084/commit/97b733953c06e4f0398ade18850f0817778255f7
    
    Clearly nobody could be bothered to upstream this patch or at minimum
    tell us :-( We only heard about this a week ago.
    
    This patch was rebased and cleaned up. Compared to the original I
    also swapped the order of the convert_in_user arguments so that they
    matched copy_in_user. It was hard to review otherwise. I also replaced
    the ALLOC_USER_SPACE/ALLOC_AND_GET by a normal function.
    
    Fixes: 6b5a9492 ("v4l: introduce string control support.")
    Signed-off-by: default avatarDaniel Mentz <danielmentz@google.com>
    Co-developed-by: default avatarHans Verkuil <hans.verkuil@cisco.com>
    Acked-by: default avatarSakari Ailus <sakari.ailus@linux.intel.com>
    Signed-off-by: default avatarHans Verkuil <hans.verkuil@cisco.com>
    Cc: <stable@vger.kernel.org>      # for v4.15 and up
    Signed-off-by: default avatarMauro Carvalho Chehab <mchehab@s-opensource.com>
    a1dfb4c4
v4l2-compat-ioctl32.c 33.8 KB