| Message ID | 20200115024431.5421-1-hmadhani@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qla2xxx: Fix unbound NVME response length | expand |
On 1/14/20 6:44 PM, Himanshu Madhani wrote: > From: Arun Easi <aeasi@marvell.com> > > On certain cases when response length is less than 32, NVME response data > is supplied inline in IOCB. This is indicated by some combination of state > flags. There was an instance when a high, and incorrect, response length was > indicated causing driver to overrun buffers. Fix this by checking and > limiting the response payload length. > > Fixes: 7401bc18d1ee3 ("scsi: qla2xxx: Add FC-NVMe command handling") > Cc: stable@vger.kernel.com > Signed-off-by: Arun Easi <aeasi@marvell.com> > Signed-off-by: Himanshu Madhani <hmadhani@marvell.com> > --- > Hi Martin, > > We discovered issue with our newer Gen7 adapter when response length > happens to be larger than 32 bytes, could result into crash. > > Please apply this to 5.5/scsi-fixes branch at your earliest convenience. > > Thanks, > Himanshu > --- > drivers/scsi/qla2xxx/qla_isr.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c > index e7bad0bfffda..90e816d13b0e 100644 > --- a/drivers/scsi/qla2xxx/qla_isr.c > +++ b/drivers/scsi/qla2xxx/qla_isr.c > @@ -1939,6 +1939,15 @@ static void qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req, > inbuf = (uint32_t *)&sts->nvme_ersp_data; > outbuf = (uint32_t *)fd->rspaddr; > iocb->u.nvme.rsp_pyld_len = le16_to_cpu(sts->nvme_rsp_pyld_len); > + if (unlikely(iocb->u.nvme.rsp_pyld_len > 32)) { > + WARN_ONCE(1, "Unexpected response payload length %u.\n", > + iocb->u.nvme.rsp_pyld_len); > + ql_log(ql_log_warn, fcport->vha, 0x5100, > + "Unexpected response payload length %u.\n", > + iocb->u.nvme.rsp_pyld_len); > + iocb->u.nvme.rsp_pyld_len = 32; > + logit = 1; > + } > iter = iocb->u.nvme.rsp_pyld_len >> 2; > for (; iter; iter--) > *outbuf++ = swab32(*inbuf++); > Please change the constant '32' into sizeof(...) or into a symbolic name. Thanks, Bart.
On 1/15/20, 10:12 AM, "Bart Van Assche" <bvanassche@acm.org> wrote:
External Email
----------------------------------------------------------------------
On 1/14/20 6:44 PM, Himanshu Madhani wrote:
> From: Arun Easi <aeasi@marvell.com>
>
> On certain cases when response length is less than 32, NVME response data
> is supplied inline in IOCB. This is indicated by some combination of state
> flags. There was an instance when a high, and incorrect, response length was
> indicated causing driver to overrun buffers. Fix this by checking and
> limiting the response payload length.
>
> Fixes: 7401bc18d1ee3 ("scsi: qla2xxx: Add FC-NVMe command handling")
> Cc: stable@vger.kernel.com
> Signed-off-by: Arun Easi <aeasi@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
> Hi Martin,
>
> We discovered issue with our newer Gen7 adapter when response length
> happens to be larger than 32 bytes, could result into crash.
>
> Please apply this to 5.5/scsi-fixes branch at your earliest convenience.
>
> Thanks,
> Himanshu
> ---
> drivers/scsi/qla2xxx/qla_isr.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index e7bad0bfffda..90e816d13b0e 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -1939,6 +1939,15 @@ static void qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req,
> inbuf = (uint32_t *)&sts->nvme_ersp_data;
> outbuf = (uint32_t *)fd->rspaddr;
> iocb->u.nvme.rsp_pyld_len = le16_to_cpu(sts->nvme_rsp_pyld_len);
> + if (unlikely(iocb->u.nvme.rsp_pyld_len > 32)) {
> + WARN_ONCE(1, "Unexpected response payload length %u.\n",
> + iocb->u.nvme.rsp_pyld_len);
> + ql_log(ql_log_warn, fcport->vha, 0x5100,
> + "Unexpected response payload length %u.\n",
> + iocb->u.nvme.rsp_pyld_len);
> + iocb->u.nvme.rsp_pyld_len = 32;
> + logit = 1;
> + }
> iter = iocb->u.nvme.rsp_pyld_len >> 2;
> for (; iter; iter--)
> *outbuf++ = swab32(*inbuf++);
>
Please change the constant '32' into sizeof(...) or into a symbolic name.
Will do that. Thanks.
Thanks,
Bart.
On Wed, 2020-01-15 at 08:12 -0800, Bart Van Assche wrote: > On 1/14/20 6:44 PM, Himanshu Madhani wrote: > > From: Arun Easi <aeasi@marvell.com> > > > > On certain cases when response length is less than 32, NVME response data > > is supplied inline in IOCB. This is indicated by some combination of state > > flags. There was an instance when a high, and incorrect, response length was > > indicated causing driver to overrun buffers. Fix this by checking and > > limiting the response payload length. > > > > Fixes: 7401bc18d1ee3 ("scsi: qla2xxx: Add FC-NVMe command handling") > > Cc: stable@vger.kernel.com > > Signed-off-by: Arun Easi <aeasi@marvell.com> > > Signed-off-by: Himanshu Madhani <hmadhani@marvell.com> > > --- > > Hi Martin, > > > > We discovered issue with our newer Gen7 adapter when response length > > happens to be larger than 32 bytes, could result into crash. > > > > Please apply this to 5.5/scsi-fixes branch at your earliest convenience. > > > > Thanks, > > Himanshu > > --- > > drivers/scsi/qla2xxx/qla_isr.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c > > index e7bad0bfffda..90e816d13b0e 100644 > > --- a/drivers/scsi/qla2xxx/qla_isr.c > > +++ b/drivers/scsi/qla2xxx/qla_isr.c > > @@ -1939,6 +1939,15 @@ static void qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req, > > inbuf = (uint32_t *)&sts->nvme_ersp_data; > > outbuf = (uint32_t *)fd->rspaddr; > > iocb->u.nvme.rsp_pyld_len = le16_to_cpu(sts->nvme_rsp_pyld_len); > > + if (unlikely(iocb->u.nvme.rsp_pyld_len > 32)) { > > + WARN_ONCE(1, "Unexpected response payload length %u.\n", > > + iocb->u.nvme.rsp_pyld_len); > > + ql_log(ql_log_warn, fcport->vha, 0x5100, > > + "Unexpected response payload length %u.\n", > > + iocb->u.nvme.rsp_pyld_len); > > + iocb->u.nvme.rsp_pyld_len = 32; > > + logit = 1; > > + } > > iter = iocb->u.nvme.rsp_pyld_len >> 2; > > for (; iter; iter--) > > *outbuf++ = swab32(*inbuf++); > > > > Please change the constant '32' into sizeof(...) or into a symbolic name. sizeof(struct nvme_fc_ersp_iu), it looks like. -Ewan > > Thanks, > > Bart. >
Hi Himanshu,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on scsi/for-next]
[also build test ERROR on mkp-scsi/for-next target/for-next v5.5-rc6 next-20200116]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Himanshu-Madhani/qla2xxx-Fix-unbound-NVME-response-length/20200116-142851
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/scsi/qla2xxx/qla_isr.c: In function 'qla24xx_nvme_iocb_entry':
>> drivers/scsi/qla2xxx/qla_isr.c:1949:4: error: 'logit' undeclared (first use in this function)
logit = 1;
^~~~~
drivers/scsi/qla2xxx/qla_isr.c:1949:4: note: each undeclared identifier is reported only once for each function it appears in
vim +/logit +1949 drivers/scsi/qla2xxx/qla_isr.c
1902
1903 static void qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req,
1904 void *tsk, srb_t *sp)
1905 {
1906 fc_port_t *fcport;
1907 struct srb_iocb *iocb;
1908 struct sts_entry_24xx *sts = (struct sts_entry_24xx *)tsk;
1909 uint16_t state_flags;
1910 struct nvmefc_fcp_req *fd;
1911 uint16_t ret = QLA_SUCCESS;
1912 uint16_t comp_status = le16_to_cpu(sts->comp_status);
1913
1914 iocb = &sp->u.iocb_cmd;
1915 fcport = sp->fcport;
1916 iocb->u.nvme.comp_status = comp_status;
1917 state_flags = le16_to_cpu(sts->state_flags);
1918 fd = iocb->u.nvme.desc;
1919
1920 if (unlikely(iocb->u.nvme.aen_op))
1921 atomic_dec(&sp->vha->hw->nvme_active_aen_cnt);
1922
1923 /*
1924 * State flags: Bit 6 and 0.
1925 * If 0 is set, we don't care about 6.
1926 * both cases resp was dma'd to host buffer
1927 * if both are 0, that is good path case.
1928 * if six is set and 0 is clear, we need to
1929 * copy resp data from status iocb to resp buffer.
1930 */
1931 if (!(state_flags & (SF_FCP_RSP_DMA | SF_NVME_ERSP))) {
1932 iocb->u.nvme.rsp_pyld_len = 0;
1933 } else if ((state_flags & SF_FCP_RSP_DMA)) {
1934 iocb->u.nvme.rsp_pyld_len = le16_to_cpu(sts->nvme_rsp_pyld_len);
1935 } else if (state_flags & SF_NVME_ERSP) {
1936 uint32_t *inbuf, *outbuf;
1937 uint16_t iter;
1938
1939 inbuf = (uint32_t *)&sts->nvme_ersp_data;
1940 outbuf = (uint32_t *)fd->rspaddr;
1941 iocb->u.nvme.rsp_pyld_len = le16_to_cpu(sts->nvme_rsp_pyld_len);
1942 if (unlikely(iocb->u.nvme.rsp_pyld_len > 32)) {
1943 WARN_ONCE(1, "Unexpected response payload length %u.\n",
1944 iocb->u.nvme.rsp_pyld_len);
1945 ql_log(ql_log_warn, fcport->vha, 0x5100,
1946 "Unexpected response payload length %u.\n",
1947 iocb->u.nvme.rsp_pyld_len);
1948 iocb->u.nvme.rsp_pyld_len = 32;
> 1949 logit = 1;
1950 }
1951 iter = iocb->u.nvme.rsp_pyld_len >> 2;
1952 for (; iter; iter--)
1953 *outbuf++ = swab32(*inbuf++);
1954 } else { /* unhandled case */
1955 ql_log(ql_log_warn, fcport->vha, 0x503a,
1956 "NVME-%s error. Unhandled state_flags of %x\n",
1957 sp->name, state_flags);
1958 }
1959
1960 fd->transferred_length = fd->payload_length -
1961 le32_to_cpu(sts->residual_len);
1962
1963 if (unlikely(comp_status != CS_COMPLETE))
1964 ql_log(ql_log_warn, fcport->vha, 0x5060,
1965 "NVME-%s ERR Handling - hdl=%x status(%x) tr_len:%x resid=%x ox_id=%x\n",
1966 sp->name, sp->handle, comp_status,
1967 fd->transferred_length, le32_to_cpu(sts->residual_len),
1968 sts->ox_id);
1969
1970 /*
1971 * If transport error then Failure (HBA rejects request)
1972 * otherwise transport will handle.
1973 */
1974 switch (comp_status) {
1975 case CS_COMPLETE:
1976 break;
1977
1978 case CS_RESET:
1979 case CS_PORT_UNAVAILABLE:
1980 case CS_PORT_LOGGED_OUT:
1981 fcport->nvme_flag |= NVME_FLAG_RESETTING;
1982 /* fall through */
1983 case CS_ABORTED:
1984 case CS_PORT_BUSY:
1985 fd->transferred_length = 0;
1986 iocb->u.nvme.rsp_pyld_len = 0;
1987 ret = QLA_ABORTED;
1988 break;
1989 case CS_DATA_UNDERRUN:
1990 break;
1991 default:
1992 ret = QLA_FUNCTION_FAILED;
1993 break;
1994 }
1995 sp->done(sp, ret);
1996 }
1997
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c index e7bad0bfffda..90e816d13b0e 100644 --- a/drivers/scsi/qla2xxx/qla_isr.c +++ b/drivers/scsi/qla2xxx/qla_isr.c @@ -1939,6 +1939,15 @@ static void qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req, inbuf = (uint32_t *)&sts->nvme_ersp_data; outbuf = (uint32_t *)fd->rspaddr; iocb->u.nvme.rsp_pyld_len = le16_to_cpu(sts->nvme_rsp_pyld_len); + if (unlikely(iocb->u.nvme.rsp_pyld_len > 32)) { + WARN_ONCE(1, "Unexpected response payload length %u.\n", + iocb->u.nvme.rsp_pyld_len); + ql_log(ql_log_warn, fcport->vha, 0x5100, + "Unexpected response payload length %u.\n", + iocb->u.nvme.rsp_pyld_len); + iocb->u.nvme.rsp_pyld_len = 32; + logit = 1; + } iter = iocb->u.nvme.rsp_pyld_len >> 2; for (; iter; iter--) *outbuf++ = swab32(*inbuf++);