Re: [PATCH v3 11/17] rdma_rxe: Address an issue with hardened user copy

[Bob Pearson <rpearsonhpe@gmail.com>]

On 8/24/20 3:52 AM, Leon Romanovsky wrote:
> On Fri, Aug 21, 2020 at 11:16:59PM -0500, Bob Pearson wrote:
>> On 8/21/20 10:32 PM, Zhu Yanjun wrote:
>>> On 8/21/2020 6:46 AM, Bob Pearson wrote:
>>>> Added a new feature to pools to let driver white list a region of
>>>> a pool object. This removes a kernel oops caused when create qp
>>>> returns the qp number so the next patch will work without errors.
>>>>
>>>> Signed-off-by: Bob Pearson <rpearson@hpe.com>
> 
> And we asked you to provide warning itself.
> 
> Thanks
> 

Thanks for your responses to this patch (11/17). I am not yet convinced that there is anything that needs fixing. If you read the code in __check_heap_object in mm/slab.c (see below) you can see that any memory reference to kernel space from the slab/slub allocator, even if it is otherwise perfectly fine, that is not contained in the usercopy region (useroffset to useroffset + usersize from the start of each object) will trigger a warning. This is intentional not a bug. If you create the cache with kmem_cache_create this region is NULL, if you use kmem_cache_create_usercopy you may set the limits on the usercopy region.

There at least two ways to mitigate this warning, set the usercopy region (whitelist it) or copy the data through some other memory (e.g. copy onto the stack and call user copy from there). I have tried both of these and they work but still you are looking for something else. Either of these changes makes rxe secure as you put it.

This user_copy warning is from drivers/infiniband/core and is referring to the qp objects. It has nothing to do with any of the other changes in the patches. It is caused by the addition of the checks below which are new to the mainline kernel.

#ifdef CONFIG_HARDENED_USERCOPY
/*
 * Rejects incorrectly sized objects and objects that are to be copied
 * to/from userspace but do not fall entirely within the containing slab
 * cache's usercopy region.
 *
 * Returns NULL if check passes, otherwise const char * to name of cache
 * to indicate an error.
 */
void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
                         bool to_user)
{
        struct kmem_cache *cachep;
        unsigned int objnr;       objnr = obj_to_index(cachep, page, (void *)ptr);
        BUG_ON(objnr >= cachep->num);

        /* Find offset within object. */
        offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);

        /* Allow address range falling entirely within usercopy region. */
        if (offset >= cachep->useroffset &&
            offset - cachep->useroffset <= cachep->usersize &&
            n <= cachep->useroffset - offset + cachep->usersize)
                return;
        if (usercopy_fallback &&
            offset <= cachep->object_size &&
            n <= cachep->object_size - offset) {
                usercopy_warn("SLAB object", cachep->name, to_user, offset, n);
                return;
        }

        usercopy_abort("SLAB object", cachep->name, to_user, offset, n);
}
#endif /* CONFIG_HARDENED_USERCOPY */