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 <[email protected]>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>
> 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 <[email protected]>
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 Cook
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.
--
Martin K. Petersen Oracle Linux Engineering
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 <[email protected]>
>
> 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 <[email protected]>
>
> Acked-by: Martin K. Petersen <[email protected]>
>
>> ---
>> 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);
>