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

[Bob Pearson <rpearsonhpe@gmail.com>]

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>
>> ---
>>   drivers/infiniband/sw/rxe/rxe_pool.c | 20 +++++++++++++++++---
>>   drivers/infiniband/sw/rxe/rxe_pool.h |  4 ++++
>>   2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
>> index 5679714827ec..374e56689d30 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_pool.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
>> @@ -40,9 +40,12 @@ struct rxe_type_info rxe_type_info[RXE_NUM_TYPES] = {
>>           .name        = "rxe-qp",
>>           .size        = sizeof(struct rxe_qp),
>>           .cleanup    = rxe_qp_cleanup,
>> -        .flags        = RXE_POOL_INDEX,
>> +        .flags        = RXE_POOL_INDEX
>> +                | RXE_POOL_WHITELIST,
>>           .min_index    = RXE_MIN_QP_INDEX,
>>           .max_index    = RXE_MAX_QP_INDEX,
>> +        .user_offset    = offsetof(struct rxe_qp, ibqp.qp_num),
>> +        .user_size    = sizeof(u32),
>>       },
>>       [RXE_TYPE_CQ] = {
>>           .name        = "rxe-cq",
>> @@ -116,10 +119,21 @@ int rxe_cache_init(void)
>>           type = &rxe_type_info[i];
>>           size = ALIGN(type->size, RXE_POOL_ALIGN);
>>           if (!(type->flags & RXE_POOL_NO_ALLOC)) {
>> -            type->cache =
>> -                kmem_cache_create(type->name, size,
>> +            if (type->flags & RXE_POOL_WHITELIST) {
>> +                type->cache =
>> +                    kmem_cache_create_usercopy(
>> +                        type->name, size,
>> +                        RXE_POOL_ALIGN,
>> +                        RXE_POOL_CACHE_FLAGS,
>> +                        type->user_offset,
>> +                        type->user_size, NULL);
>> +            } else {
>> +                type->cache =
>> +                    kmem_cache_create(type->name, size,
>>                             RXE_POOL_ALIGN,
>>                             RXE_POOL_CACHE_FLAGS, NULL);
>> +            }
>> +
>>               if (!type->cache) {
>>                   pr_err("Unable to init kmem cache for %s\n",
>>                          type->name);
>> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h
>> index 664153bf9392..fc5b584a8137 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_pool.h
>> +++ b/drivers/infiniband/sw/rxe/rxe_pool.h
>> @@ -17,6 +17,7 @@ enum rxe_pool_flags {
>>       RXE_POOL_INDEX        = BIT(1),
>>       RXE_POOL_KEY        = BIT(2),
>>       RXE_POOL_NO_ALLOC    = BIT(4),
>> +    RXE_POOL_WHITELIST    = BIT(5),
>>   };
>>     enum rxe_elem_type {
>> @@ -44,6 +45,9 @@ struct rxe_type_info {
>>       u32            min_index;
>>       size_t            key_offset;
>>       size_t            key_size;
>> +    /* for white listing where necessary */
> 
> s/where/when
> 
> 
>> +    unsigned int        user_offset;
>> +    unsigned int        user_size;
>>       struct kmem_cache    *cache;
>>   };
>>   
> 
> 
The reason for this change is that every time I do anything with rdma_rxe on current head of tree I get a kernel oops with a warning that there is a bad or missing white list. I traced this back to the user_copy routine which (recently) decided that when you copy just a part of a kernel memory object stored in a kmem cache that this represented a risk of leaking information from the kernel to user space. For the QP object the qp_num is copied back to user space in the user API. They also provided a new kmem_ccache_create_usercopy call that allows you to specify a 'whitelisted' portion of each object with an offset and length. So I just made it a feature of pools since it may come up again instead of treating QPs differently that all the other objects. This is part of a general program to harden the Linux kernel.
You can see the change to rxe_cache_init in the same file. Perhaps just dropping the comment would address the concern. See an earlier post I made with a pointer to an article in lwn describing the changes to the kernel.