Potential NULL pointer dereference in drivers/infiniband/core

Potential NULL pointer dereference in drivers/infiniband/core

[Shaobo]

Dear developers,

My name is Shaobo He and I am a graduate student at University of Utah. 
I am applying a static analysis tool to the Linux device drivers and got 
an error trace of null pointer dereference in drivers/infiniband/core 
starting from function `ucma_accept`: it calls `rdma_accept` with the 
second argument being NULL. In `rdma_accept`, `cma_accept_iw` is called 
with the second argument also being NULL. Then in `cma_accept_iw`, 
`cma_modify_qp_rtr` can return 0 if `id_priv->id.qp` is NULL, which can 
be suggested by an if statement in `rdma_accept`. Finally, the second 
argument `conn_param` of `cma_accept_iw` gets dereferenced. As you can 
see, the error trace is only plausible since it depends on certain 
conditions. Therefore, I was wondering if you could confirm it.

Thanks for your time. I am looking forward to your reply.

Best,
Shaobo
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Potential NULL pointer dereference in drivers/infiniband/core

[Yuval Shaia]

On Mon, Feb 20, 2017 at 11:23:41PM -0700, Shaobo wrote:
> Dear developers,
> 
> My name is Shaobo He and I am a graduate student at University of
> Utah. I am applying a static analysis tool to the Linux device
> drivers and got an error trace of null pointer dereference in
> drivers/infiniband/core starting from function `ucma_accept`: it
> calls `rdma_accept` with the second argument being NULL. In
> `rdma_accept`, `cma_accept_iw` is called with the second argument
> also being NULL. Then in `cma_accept_iw`, `cma_modify_qp_rtr` can
> return 0 if `id_priv->id.qp` is NULL, which can be suggested by an
> if statement in `rdma_accept`. Finally, the second argument
> `conn_param` of `cma_accept_iw` gets dereferenced. As you can see,
> the error trace is only plausible since it depends on certain
> conditions. Therefore, I was wondering if you could confirm it.
> 
> Thanks for your time. I am looking forward to your reply.

Suggesting to prepare a patch and ask for review.
I believe the issue will get faster attention this way.

> 
> Best,
> Shaobo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Potential NULL pointer dereference in drivers/infiniband/core

[Hefty, Sean]

Steve,

Can you look at the iWarp code flow described below?

> My name is Shaobo He and I am a graduate student at University of Utah.
> I am applying a static analysis tool to the Linux device drivers and
> got an error trace of null pointer dereference in
> drivers/infiniband/core starting from function `ucma_accept`: it calls
> `rdma_accept` with the second argument being NULL. In `rdma_accept`,
> `cma_accept_iw` is called with the second argument also being NULL.
> Then in `cma_accept_iw`, `cma_modify_qp_rtr` can return 0 if `id_priv-
> >id.qp` is NULL, which can be suggested by an if statement in
> `rdma_accept`. Finally, the second argument `conn_param` of
> `cma_accept_iw` gets dereferenced. As you can see, the error trace is
> only plausible since it depends on certain conditions. Therefore, I was
> wondering if you could confirm it.

Based on a quick look, this looks like it's at least a problem in how conn_param is verified.  IB allows conn_param to be optional, whereas iWarp requires it.  Since this is coming from user space, we can crash.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Potential NULL pointer dereference in drivers/infiniband/core

[Steve Wise]

> 
> Steve,
> 
> Can you look at the iWarp code flow described below?

sure:

> 
> > My name is Shaobo He and I am a graduate student at University of Utah.
> > I am applying a static analysis tool to the Linux device drivers and
> > got an error trace of null pointer dereference in
> > drivers/infiniband/core starting from function `ucma_accept`: it calls
> > `rdma_accept` with the second argument being NULL. In `rdma_accept`,
> > `cma_accept_iw` is called with the second argument also being NULL.
> > Then in `cma_accept_iw`, `cma_modify_qp_rtr` can return 0 if `id_priv-
> > >id.qp` is NULL, which can be suggested by an if statement in
> > `rdma_accept`. Finally, the second argument `conn_param` of
> > `cma_accept_iw` gets dereferenced. As you can see, the error trace is
> > only plausible since it depends on certain conditions. Therefore, I was
> > wondering if you could confirm it.
> 
> Based on a quick look, this looks like it's at least a problem in how
conn_param is
> verified.  IB allows conn_param to be optional, whereas iWarp requires it.
Since
> this is coming from user space, we can crash.
> 

Agreed.  cma_accept_iw() needs to either fail the accept for a NULL conn_param,
or generate values for the iw_param struct if conn_param is NULL.  I suggest the
former.  Something like:


diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 3e70a9c..c377afc 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -3583,6 +3583,9 @@ static int cma_accept_iw(struct rdma_id_private *id_priv,
        struct iw_cm_conn_param iw_param;
        int ret;

+       if (!conn_param)
+               return -EINVAL;
+
        ret = cma_modify_qp_rtr(id_priv, conn_param);
        if (ret)
                return ret;


---
Steve.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Potential NULL pointer dereference in drivers/infiniband/core

[Hefty, Sean]

> diff --git a/drivers/infiniband/core/cma.c
> b/drivers/infiniband/core/cma.c
> index 3e70a9c..c377afc 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -3583,6 +3583,9 @@ static int cma_accept_iw(struct rdma_id_private
> *id_priv,
>         struct iw_cm_conn_param iw_param;
>         int ret;
> 
> +       if (!conn_param)
> +               return -EINVAL;
> +
>         ret = cma_modify_qp_rtr(id_priv, conn_param);
>         if (ret)
>                 return ret;

Can you re-submit as a formal patch?  Feel free to include Acked-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This should go into stable.  Thanks!  And thanks Shaobo!
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Potential NULL pointer dereference in drivers/infiniband/core

[Steve Wise]

> 
> Can you re-submit as a formal patch?  Feel free to include Acked-by: Sean
Hefty
> <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This should go into stable.  Thanks!  And thanks Shaobo!

Will do.  

Steve.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html