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