On Mon, Oct 28, 2019 at 10:13:19AM -0300, Jason Gunthorpe wrote:
> On Sun, Oct 20, 2019 at 10:15:59AM +0300, Leon Romanovsky wrote:
> > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > index daf811abf40a..e66366de11e9 100644
> > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > @@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
> > case IB_CM_REJ_RECEIVED:
> > srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
> > event->private_data,
> > - IB_CM_REJ_PRIVATE_DATA_SIZE);
> > + event->private_data_len);
>
> So, I took a look and found a heck of a lot more places assuming the
> size of private data that really should be checked if we are going to
> introduce a buffer length here.
But we are not interested to make it dynamic, "private_data_len" has
constant size according to IBTA spec, I just don't want users to be
aware of this.
Why should we add the below checks if it wasn't before?
>
> This is the first couple I noticed, but there were many many more and
> they all should be handled..
>
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -2677,9 +2677,14 @@ static void srp_ib_cm_rej_handler(struct ib_cm_id *cm_id,
> break;
>
> case IB_CM_REJ_CONSUMER_DEFINED:
> + if (event->private_data_len < sizeof(struct srp_login_rej)) {
> + ch->status = -ECONNRESET;
> + break;
> + }
> +
>
> --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> @@ -985,12 +985,15 @@ static int ipoib_cm_rep_handler(struct ib_cm_id *cm_id,
> {
> struct ipoib_cm_tx *p = cm_id->context;
> struct ipoib_dev_priv *priv = ipoib_priv(p->dev);
> struct ipoib_cm_data *data = event->private_data;
> struct sk_buff_head skqueue;
> struct ib_qp_attr qp_attr;
> int qp_attr_mask, ret;
> struct sk_buff *skb;
>
> + if (event->private_data_len < sizeof(*data))
> + return -EINVAL;
> +
>
>
> Thanks,
> Jason
