| Message ID | 20210727205855.411487-37-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
Kees, > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memset(), avoid intentionally writing across > neighboring fields. > > Instead of writing beyond the end of evt_struct->iu.srp.cmd, target the > upper union (evt_struct->iu.srp) instead, as that's what is being wiped. > > Signed-off-by: Kees Cook <keescook@chromium.org> Orthogonal to your change, it wasn't immediately obvious to me that SRP_MAX_IU_LEN was the correct length to use for an srp_cmd. However, I traversed the nested unions and it does look OK. For good measure I copied Tyrel and Brian. Acked-by: Martin K. Petersen <martin.petersen@oracle.com> > --- > drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c > index e6a3eaaa57d9..7e8beb42d2d3 100644 > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c > @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, > return SCSI_MLQUEUE_HOST_BUSY; > > /* Set up the actual SRP IU */ > + memset(&evt_struct->iu.srp, 0x00, SRP_MAX_IU_LEN); > srp_cmd = &evt_struct->iu.srp.cmd; > - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > srp_cmd->opcode = SRP_CMD; > memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); > int_to_scsilun(lun, &srp_cmd->lun);
On Tue, Jul 27, 2021 at 09:39:39PM -0400, Martin K. Petersen wrote: > > Kees, > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memset(), avoid intentionally writing across > > neighboring fields. > > > > Instead of writing beyond the end of evt_struct->iu.srp.cmd, target the > > upper union (evt_struct->iu.srp) instead, as that's what is being wiped. > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Orthogonal to your change, it wasn't immediately obvious to me that > SRP_MAX_IU_LEN was the correct length to use for an srp_cmd. However, I > traversed the nested unions and it does look OK. Yeah, I had the same fun. Maybe I should add a BUILD_BUG_ON() here to help illustrate the relationship? I did that in a few other places where the equalities weren't very clear. For example, change it to: + BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN); + memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp)); srp_cmd = &evt_struct->iu.srp.cmd; - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > > For good measure I copied Tyrel and Brian. > > Acked-by: Martin K. Petersen <martin.petersen@oracle.com> For the moment, I'll leave the patch as-is unless you prefer having the BUILD_BUG_ON(). :) Thanks! -Kees > > > --- > > drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c > > index e6a3eaaa57d9..7e8beb42d2d3 100644 > > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c > > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c > > @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, > > return SCSI_MLQUEUE_HOST_BUSY; > > > > /* Set up the actual SRP IU */ > > + memset(&evt_struct->iu.srp, 0x00, SRP_MAX_IU_LEN); > > srp_cmd = &evt_struct->iu.srp.cmd; > > - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > > srp_cmd->opcode = SRP_CMD; > > memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); > > int_to_scsilun(lun, &srp_cmd->lun); > > -- > Martin K. Petersen Oracle Linux Engineering
Kees, > For example, change it to: > > + BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN); > + memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp)); > srp_cmd = &evt_struct->iu.srp.cmd; > - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > For the moment, I'll leave the patch as-is unless you prefer having > the BUILD_BUG_ON(). :) I'm OK with the BUILD_BUG_ON(). Hopefully Tyrel or Brian will chime in.
On 7/27/21 6:39 PM, Martin K. Petersen wrote: > > Kees, > >> In preparation for FORTIFY_SOURCE performing compile-time and run-time >> field bounds checking for memset(), avoid intentionally writing across >> neighboring fields. >> >> Instead of writing beyond the end of evt_struct->iu.srp.cmd, target the >> upper union (evt_struct->iu.srp) instead, as that's what is being wiped. >> >> Signed-off-by: Kees Cook <keescook@chromium.org> > > Orthogonal to your change, it wasn't immediately obvious to me that > SRP_MAX_IU_LEN was the correct length to use for an srp_cmd. However, I > traversed the nested unions and it does look OK. > > For good measure I copied Tyrel and Brian. LGTM Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com> > > Acked-by: Martin K. Petersen <martin.petersen@oracle.com> > >> --- >> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >> index e6a3eaaa57d9..7e8beb42d2d3 100644 >> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c >> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c >> @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, >> return SCSI_MLQUEUE_HOST_BUSY; >> >> /* Set up the actual SRP IU */ >> + memset(&evt_struct->iu.srp, 0x00, SRP_MAX_IU_LEN); >> srp_cmd = &evt_struct->iu.srp.cmd; >> - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); >> srp_cmd->opcode = SRP_CMD; >> memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); >> int_to_scsilun(lun, &srp_cmd->lun); >
On 7/28/21 8:35 PM, Martin K. Petersen wrote: > > Kees, > >> For example, change it to: >> >> + BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN); >> + memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp)); >> srp_cmd = &evt_struct->iu.srp.cmd; >> - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > >> For the moment, I'll leave the patch as-is unless you prefer having >> the BUILD_BUG_ON(). :) > > I'm OK with the BUILD_BUG_ON(). Hopefully Tyrel or Brian will chime in. > All the other srp structs are at most 64 bytes and the size of the union is explicitly set to SRP_MAX_IU_LEN by the last field of the union. union srp_iu { struct srp_login_req login_req; struct srp_login_rsp login_rsp; struct srp_login_rej login_rej; struct srp_i_logout i_logout; struct srp_t_logout t_logout; struct srp_tsk_mgmt tsk_mgmt; struct srp_cmd cmd; struct srp_rsp rsp; u8 reserved[SRP_MAX_IU_LEN]; }; So, in my mind if SRP_MAX_IU_LEN ever changes so does the size of the union making the BUILD_BUG_ON() superfluous. But it doesn't really hurt anything either. -Tyrel
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c index e6a3eaaa57d9..7e8beb42d2d3 100644 --- a/drivers/scsi/ibmvscsi/ibmvscsi.c +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, return SCSI_MLQUEUE_HOST_BUSY; /* Set up the actual SRP IU */ + memset(&evt_struct->iu.srp, 0x00, SRP_MAX_IU_LEN); srp_cmd = &evt_struct->iu.srp.cmd; - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); srp_cmd->opcode = SRP_CMD; memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); int_to_scsilun(lun, &srp_cmd->lun);
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memset(), avoid intentionally writing across neighboring fields. Instead of writing beyond the end of evt_struct->iu.srp.cmd, target the upper union (evt_struct->iu.srp) instead, as that's what is being wiped. Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)