Re: [bug report] rdma/siw: queue pair methods

From: Doug Ledford <dledford@redhat.com>
To: Bernard Metzler <BMT@zurich.ibm.com>,
	Dan Carpenter <dan.carpenter@oracle.com>
Cc: linux-rdma@vger.kernel.org
Subject: Re: [bug report] rdma/siw: queue pair methods
Date: Mon, 29 Jul 2019 13:36:31 -0400	[thread overview]
Message-ID: <9c4975b8fed483c0911f27f4b90c0566962ada3d.camel@redhat.com> (raw)
In-Reply-To: <OF61E386ED.49A73798-ON00258444.003BD6A6-00258444.003CC8D9@notes.na.collabserv.com>

[-- Attachment #1: Type: text/plain, Size: 4172 bytes --]

On Sat, 2019-07-27 at 11:03 +0000, Bernard Metzler wrote:
> -----"Dan Carpenter" <dan.carpenter@oracle.com> wrote: -----
> 
> > To: bmt@zurich.ibm.com
> > From: "Dan Carpenter" <dan.carpenter@oracle.com>
> > Date: 07/26/2019 10:11AM
> > Cc: linux-rdma@vger.kernel.org
> > Subject: [EXTERNAL] [bug report] rdma/siw: queue pair methods
> > 
> > Hello Bernard Metzler,
> > 
> > The patch f29dd55b0236: "rdma/siw: queue pair methods" from Jun 20,
> > 2019, leads to the following static checker warning:
> > 
> > 	drivers/infiniband/sw/siw/siw_qp.c:226 siw_qp_enable_crc()
> > 	warn: variable dereferenced before check 'siw_crypto_shash' (see
> > line 223)
> > 
> > drivers/infiniband/sw/siw/siw_qp.c
> >   219  static int siw_qp_enable_crc(struct siw_qp *qp)
> >   220  {
> >   221          struct siw_rx_stream *c_rx = &qp->rx_stream;
> >   222          struct siw_iwarp_tx *c_tx = &qp->tx_ctx;
> >   223          int size = crypto_shash_descsize(siw_crypto_shash) +
> >                                                 ^^^^^^^^^^^^^^^^
> > Dereferenced inside function.
> > 
> >   224                          sizeof(struct shash_desc);
> >   225  
> >   226          if (siw_crypto_shash == NULL)
> >                    ^^^^^^^^^^^^^^^^^^^^^^^^
> > Checked too late.
> > 
> >   227                  return -ENOENT;
> >   228  
> >   229          c_tx->mpa_crc_hd = kzalloc(size, GFP_KERNEL);
> >   230          c_rx->mpa_crc_hd = kzalloc(size, GFP_KERNEL);
> >   231          if (!c_tx->mpa_crc_hd || !c_rx->mpa_crc_hd) {
> >   232                  kfree(c_tx->mpa_crc_hd);
> >   233                  kfree(c_rx->mpa_crc_hd);
> >   234                  c_tx->mpa_crc_hd = NULL;
> >   235                  c_rx->mpa_crc_hd = NULL;
> >   236                  return -ENOMEM;
> >   237          }
> >   238          c_tx->mpa_crc_hd->tfm = siw_crypto_shash;
> >   239          c_rx->mpa_crc_hd->tfm = siw_crypto_shash;
> >   240  
> >   241          return 0;
> >   242  }
> > 
> > regards,
> > dan carpenter
> > 
> > 
> 
> Hi Dan,
> many thanks for catching this one! The fix of course is simple:
> 

Hi Bernard,

This patch was ignored by patchworks for some reason.  If I hadn't
noticed that it was here, but not in patchworks and also not applied
previously by Jason, it would have been missed entirely.  I suspect it's
because the patch was embedded in a reply, but I'm not sure as that
normally seems to work.  In any case, I might suggest next time you
reply to the bug report that you have a fix, and then use git send-email 
to send the patch, just to be on the safe side in terms of things
getting lost.

With all that said, applied to for-rc along with some fix ups to the log
message (added Reported-by: and Fixes: tags).

> From c13b5da99aea7766a61aabe33e9943618f4505cf Mon Sep 17 00:00:00 2001
> From: Bernard Metzler <bmt@zurich.ibm.com>
> Date: Sat, 27 Jul 2019 12:38:32 +0200
> Subject: [PATCH] Do not dereference 'siw_crypto_shash' before checking
> 
> Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
> ---
>  drivers/infiniband/sw/siw/siw_qp.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_qp.c
> b/drivers/infiniband/sw/siw/siw_qp.c
> index 11383d9f95ef..e27bd5b35b96 100644
> --- a/drivers/infiniband/sw/siw/siw_qp.c
> +++ b/drivers/infiniband/sw/siw/siw_qp.c
> @@ -220,12 +220,14 @@ static int siw_qp_enable_crc(struct siw_qp *qp)
>  {
>  	struct siw_rx_stream *c_rx = &qp->rx_stream;
>  	struct siw_iwarp_tx *c_tx = &qp->tx_ctx;
> -	int size = crypto_shash_descsize(siw_crypto_shash) +
> -			sizeof(struct shash_desc);
> +	int size;
>  
>  	if (siw_crypto_shash == NULL)
>  	return -ENOENT;
>  
> +	size = crypto_shash_descsize(siw_crypto_shash) +
> +		sizeof(struct shash_desc);
> +
>  	c_tx->mpa_crc_hd = kzalloc(size, GFP_KERNEL);
>  	c_rx->mpa_crc_hd = kzalloc(size, GFP_KERNEL);
>  	if (!c_tx->mpa_crc_hd || !c_rx->mpa_crc_hd) {

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]