[02/14] scsi: prevent data transfer overflow

Avoid sending more than 2GB of data, as that can cause overflows
in int32_t variables.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |   38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

Am 04.05.2012 10:45, schrieb Paolo Bonzini:
> Avoid sending more than 2GB of data, as that can cause overflows
> in int32_t variables.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/scsi-bus.c |   38 ++++++++++++++++++++++++++------------
>   1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index dbdb99c..c29a4ae 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -239,6 +239,18 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
>       return res;
>   }
>
> +static int32_t scsi_invalid_field(SCSIRequest *req, uint8_t *buf)
> +{
> +    scsi_req_build_sense(req, SENSE_CODE(INVALID_FIELD));
> +    scsi_req_complete(req, CHECK_CONDITION);
> +    return 0;
> +}
> +
> +static const struct SCSIReqOps reqops_invalid_field = {
> +    .size         = sizeof(SCSIRequest),
> +    .send_command = scsi_invalid_field
> +};
> +
>   /* SCSIReqOps implementation for invalid commands.  */
>
>   static int32_t scsi_invalid_command(SCSIRequest *req, uint8_t *buf)
> @@ -517,18 +529,20 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
>                                         cmd.lba);
>           }
>
> -        if ((d->unit_attention.key == UNIT_ATTENTION ||
> -             bus->unit_attention.key == UNIT_ATTENTION)&&
> -            (buf[0] != INQUIRY&&
> -             buf[0] != REPORT_LUNS&&
> -             buf[0] != GET_CONFIGURATION&&
> -             buf[0] != GET_EVENT_STATUS_NOTIFICATION&&
> -
> -             /*
> -              * If we already have a pending unit attention condition,
> -              * report this one before triggering another one.
> -              */
> -             !(buf[0] == REQUEST_SENSE&&  d->sense_is_ua))) {
> +        if (cmd.xfer>  INT32_MAX) {
> +            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, hba_private);

WARNING: line over 80 characters
#54: FILE: hw/scsi-bus.c:533:
+            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, 
hba_private);

total: 0 errors, 1 warnings, 50 lines checked

0002-scsi-prevent-data-transfer-overflow.patch has style problems, 
please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.


> +        } else if ((d->unit_attention.key == UNIT_ATTENTION ||
> +                   bus->unit_attention.key == UNIT_ATTENTION)&&
> +                  (buf[0] != INQUIRY&&
> +                   buf[0] != REPORT_LUNS&&
> +                   buf[0] != GET_CONFIGURATION&&
> +                   buf[0] != GET_EVENT_STATUS_NOTIFICATION&&
> +
> +                   /*
> +                    * If we already have a pending unit attention condition,
> +                    * report this one before triggering another one.
> +                    */
> +                   !(buf[0] == REQUEST_SENSE&&  d->sense_is_ua))) {
>               req = scsi_req_alloc(&reqops_unit_attention, d, tag, lun,
>                                    hba_private);
>           } else if (lun != d->lun ||

Il 04/05/2012 18:28, Stefan Weil ha scritto:
>>
>> -             !(buf[0] == REQUEST_SENSE&&  d->sense_is_ua))) {
>> +        if (cmd.xfer>  INT32_MAX) {
>> +            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun,
>> hba_private);
> 
> WARNING: line over 80 characters
> #54: FILE: hw/scsi-bus.c:533:
> +            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun,
> hba_private);
> 
> total: 0 errors, 1 warnings, 50 lines checked
> 
> 0002-scsi-prevent-data-transfer-overflow.patch has style problems,
> please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.

It's a warning for a reason...

Paolo

Am 04.05.2012 18:29, schrieb Paolo Bonzini:
> Il 04/05/2012 18:28, Stefan Weil ha scritto:
>>> - !(buf[0] == REQUEST_SENSE&& d->sense_is_ua))) { + if (cmd.xfer> 
>>> INT32_MAX) { + req = scsi_req_alloc(&reqops_invalid_field, d, tag, 
>>> lun, hba_private); 
>> WARNING: line over 80 characters #54: FILE: hw/scsi-bus.c:533: + req 
>> = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, hba_private); 
>> total: 0 errors, 1 warnings, 50 lines checked 
>> 0002-scsi-prevent-data-transfer-overflow.patch has style problems, 
>> please review. If any of these errors are false positives report them 
>> to the maintainer, see CHECKPATCH in MAINTAINERS. 
> It's a warning for a reason... Paolo

That's ok. It was just funny that a patch which prevents
data transfer overflow introduces a line overflow :-)

The tabs in patches 6 and 7 are more problematic.
Although they are not at the start of the line, tabs
should be completely avoided (that's my personal
understanding of QEMU's coding rules).

Existing code which violates that rule is no reason to
add more code with tabs. Exceptions should be possible
for code imported from other projects, maybe also
when existing code with tabs is extended.

But what is even more important than those tabs is
getting the fixes in qemu 1.1. I pulled them just
to see whether my scsi problem was fixed, and it's
git and checkpatch.pl which complains, not me :-)

Cheers,
Stefan

Il 04/05/2012 18:51, Stefan Weil ha scritto:
> The tabs in patches 6 and 7 are more problematic.
> Although they are not at the start of the line, tabs
> should be completely avoided (that's my personal
> understanding of QEMU's coding rules).

ok, I pushed an updated patchset to scsi-next (commit
68bd348ade453821fd5378479e6718e69bf181f1).

Paolo