Re: [PATCH for-rc] RDMA/bnxt_re: Fix stack-out-of-bounds in bnxt_qplib_rcfw_send_message

[Selvin Xavier <selvin.xavier@broadcom.com>]

On Wed, Aug 21, 2019 at 4:41 PM Selvin Xavier
<selvin.xavier@broadcom.com> wrote:
>
> On Tue, Aug 20, 2019 at 10:43 PM Doug Ledford <dledford@redhat.com> wrote:
> >
> > On Mon, 2019-08-19 at 13:42 +0530, Selvin Xavier wrote:
> > > On Fri, Aug 16, 2019 at 5:30 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > > On Fri, Aug 16, 2019 at 10:22:25AM +0530, Selvin Xavier wrote:
> > > > > On Thu, Aug 15, 2019 at 6:37 PM Jason Gunthorpe <jgg@ziepe.ca>
> > > > > wrote:
> > > > > > On Thu, Aug 15, 2019 at 04:44:37AM -0700, Selvin Xavier wrote:
> > > > > > > @@ -583,7 +584,7 @@ int bnxt_qplib_create_srq(struct
> > > > > > > bnxt_qplib_res *res,
> > > > > > >       req.eventq_id = cpu_to_le16(srq->eventq_hw_ring_id);
> > > > > > >
> > > > > > >       rc = bnxt_qplib_rcfw_send_message(rcfw, (void *)&req,
> > > > > > > -                                       (void *)&resp, NULL,
> > > > > > > 0);
> > > > > > > +                                       (void *)&resp,
> > > > > > > sizeof(req), NULL, 0);
> > > > > >
> > > > > > I really don't like seeing casts to void * in code. Why can't
> > > > > > you
> > > > > > properly embed the header in the structs??
> > > > > Is your objection only in casting to void * or you dont like any
> > > > > casting here?
> > > >
> > > > Explicit cast to void to erase the type is a particularly bad habit
> > > > that I don't like to see.
> > > >
> > > > You'd be better to make the send_message accept void * and the cast
> > > > inside to the header.
> > > >
> > > Ok. Will make this change. But this looks like a for-next item..
> > > right?
> > > If so, can you take this patch as is for RC? I will post the change
> > > mentioned for for-next separately.
> >
> > This patch is a lot of lines of churn.  What creating an array of sizes
> > such that you could use the cmd value as an index into the array.  Then
> > you'd only need to modify a header file to define the array, and the
> > send function to lookup the length of the command based upon the command
> > value itself.  Far fewer lines of churn, especially if you are possibly
> > going to replace this fix in for-next.
> Ok. I will work on an optimized solution for this and post it.
> Thanks,
>
Doug,
Instead of using an arrary to handle this,  I am posting a patch which
is taking the
exact command size in the structure passed to
bnxt_qplib_rcfw_send_message. Before
sending the command to FW, we will modify the value to the way FW expects.

Thanks,
Selvin