Re: Potential NULL pointer dereference in drivers/infiniband/hw/mlx4/qp.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2/25/19 4:18 PM, Shaobo wrote:
> Hello everyone,
> 
> In function `_mlx4_ib_create_qp`, `create_qp_common` is called in case `IB_QPT_UD` with qp being NULL. In this function, there exists a code segment as follows,
> 
> ```
>                  if (init_attr->create_flags & IB_QP_CREATE_NETIF_QP) {
>             if (dev->steering_support ==
>                 MLX4_STEERING_MODE_DEVICE_MANAGED)
>                 qp->flags |= MLX4_IB_QP_NETIF;
>             else
>                 goto err;

This is an error in the code, should be fixed.

>         }
> ```
> So it's possible that the function returns with `err` being 0 (previously set by `set_rq_size`). This leads to NULL pointer dereference at line 1515 in qp.c because `qp` remains to be NULL while `err` returned is 0. I was wondering if some `if` statement checking the `err` variable is required here or it's omitted by purpose. Please let me know if it makes sense.

The way the code looks today, this error can't be hit, I'll explain:
MLX4_IB_QP_NETIF = IB_QP_CREATE_NETIF_QP (see drivers/infiniband/hw/mlx4/mlx4_ib.h for that).

The only place in the kernel that passes this flag is in:
drivers/infiniband/ulp/ipoib/ipoib_verbs.c
*I care only about the kernel because of this check in _mlx4_ib_create_qp()
        /*
         * We only support LSO, vendor flag1, and multicast loopback blocking,
         * and only for kernel UD QPs.
         */
        if (init_attr->create_flags & ~(MLX4_IB_QP_LSO |
                                        MLX4_IB_QP_BLOCK_MULTICAST_LOOPBACK |
                                        MLX4_IB_SRIOV_TUNNEL_QP |
                                        MLX4_IB_SRIOV_SQP |
                                        MLX4_IB_QP_NETIF |
                                        MLX4_IB_QP_CREATE_ROCE_V2_GSI))
                return ERR_PTR(-EINVAL);

Ok, so lets see what IPoIB does (ipoib_verbs.c):
        if (priv->hca_caps & IB_DEVICE_MANAGED_FLOW_STEERING)
                init_attr.create_flags |= IB_QP_CREATE_NETIF_QP;

hca_caps is set here (ipoib_main.c:1841):
priv->hca_caps = priv->ca->attrs.device_cap_flags;

priv->ca = ib device the IPoIB interface is created on.

so back to mlx4, lets see when it sets IB_DEVICE_MANAGED_FLOW_STEERING, this is done in
drivers/infiniband/hw/mlx4/main.c:506
        if (dev->steering_support == MLX4_STEERING_MODE_DEVICE_MANAGED)
                props->device_cap_flags |= IB_DEVICE_MANAGED_FLOW_STEERING;


so as the code stands today, it can't lead to a null pointer dereference but the reason way
isn't that clear :)  

> 
> Best,
> Shaobo

Mark




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux