| Message ID | 20230607113843.37185-6-njavali@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/8] qla2xxx: klocwork - Array index may go out of bound | expand |
> On Jun 7, 2023, at 4:38 AM, Nilesh Javali <njavali@marvell.com> wrote: > > From: Quinn Tran <qutran@marvell.com> > > Klocwork warning: Buffer Overflow - Array Index Out of Bounds > > Driver uses fc_els_flogi to calculate size of buffer. > The actual buffer is nested inside of fc_els_flogi > which is smaller. > > Replace structure name to allow proper size calculation. > > Cc: stable@vger.kernel.org > Signed-off-by: Quinn Tran <qutran@marvell.com> > Signed-off-by: Nilesh Javali <njavali@marvell.com> > --- > drivers/scsi/qla2xxx/qla_init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index 0df6eae7324e..b0225f6f3221 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -5549,7 +5549,7 @@ static void qla_get_login_template(scsi_qla_host_t *vha) > __be32 *q; > > memset(ha->init_cb, 0, ha->init_cb_size); > - sz = min_t(int, sizeof(struct fc_els_flogi), ha->init_cb_size); > + sz = min_t(int, sizeof(struct fc_els_csp), ha->init_cb_size); > rval = qla24xx_get_port_login_templ(vha, ha->init_cb_dma, > ha->init_cb, sz); > if (rval != QLA_SUCCESS) { > -- > 2.23.1 > Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
On Wed, Jun 07, 2023 at 05:08:40PM +0530, Nilesh Javali wrote: > From: Quinn Tran <qutran@marvell.com> > > Klocwork warning: Buffer Overflow - Array Index Out of Bounds > > Driver uses fc_els_flogi to calculate size of buffer. > The actual buffer is nested inside of fc_els_flogi > which is smaller. To be honest, I don't really understand where either fc_els_flogi or fc_els_csp structs are involved in this. Is that the source buffer? We are copying data to ha->init_cb which is type init_cb_t. The names "ha->init_cb" and "ha->init_cb_size" match and how the size is set (in qla2x00_probe_one()) is like this: ha->init_cb_size = sizeof(init_cb_t); ha->init_cb_size = sizeof(struct mid_init_cb_24xx); ha->init_cb_size = sizeof(struct mid_init_cb_24xx); ha->init_cb_size = sizeof(struct mid_init_cb_81xx); ha->init_cb_size = sizeof(struct mid_init_cb_81xx); ha->init_cb_size = sizeof(struct mid_init_cb_81xx); ha->init_cb_size = sizeof(struct mid_init_cb_81xx); ha->init_cb_size = sizeof(struct mid_init_cb_81xx); ha->init_cb_size = sizeof(struct mid_init_cb_81xx); I don't understand the Klocwork warning either... > > Replace structure name to allow proper size calculation. > > Cc: stable@vger.kernel.org > Signed-off-by: Quinn Tran <qutran@marvell.com> > Signed-off-by: Nilesh Javali <njavali@marvell.com> > --- > drivers/scsi/qla2xxx/qla_init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index 0df6eae7324e..b0225f6f3221 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -5549,7 +5549,7 @@ static void qla_get_login_template(scsi_qla_host_t *vha) > __be32 *q; > > memset(ha->init_cb, 0, ha->init_cb_size); ^^^^^^^^^^^ > - sz = min_t(int, sizeof(struct fc_els_flogi), ha->init_cb_size); > + sz = min_t(int, sizeof(struct fc_els_csp), ha->init_cb_size); It's strange that we are checking min_t() after a memset(). Normally you check first then memset. > rval = qla24xx_get_port_login_templ(vha, ha->init_cb_dma, > ha->init_cb, sz); ^^^^^^^^^^^^^^^^ sz is used as the size of ha->init_cb here, and then again at the the end of the function. regards, dan carpenter
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index 0df6eae7324e..b0225f6f3221 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -5549,7 +5549,7 @@ static void qla_get_login_template(scsi_qla_host_t *vha) __be32 *q; memset(ha->init_cb, 0, ha->init_cb_size); - sz = min_t(int, sizeof(struct fc_els_flogi), ha->init_cb_size); + sz = min_t(int, sizeof(struct fc_els_csp), ha->init_cb_size); rval = qla24xx_get_port_login_templ(vha, ha->init_cb_dma, ha->init_cb, sz); if (rval != QLA_SUCCESS) {