Ok,
Please find attached updated patch.
The test tool starts to get some semi-decent coverage now, but is always needed.
The test for example to perform a READ16 from LBA=0x8000000000000000.
It seems that almost all targets I have tested with
had the same similar bug where the command is actually successful but
it returns the data for LBA 0 instead :-)
regards
ronnie sahlberg
On Sun, Jun 17, 2012 at 11:13 AM, FUJITA Tomonori
<fujita.tomonori@xxxxxxxxxxxxx> wrote:
> On Sun, 17 Jun 2012 09:39:00 +1000
> ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
>
>> On Sun, Jun 17, 2012 at 8:53 AM, FUJITA Tomonori
>> <fujita.tomonori@xxxxxxxxxxxxx> wrote:
>>> On Mon, 4 Jun 2012 19:10:12 +1000
>>> ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
>>>
>>>> From efc1f1720ad9a00e69f7935a001df906e609afe5 Mon Sep 17 00:00:00 2001
>>>> From: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>>>> Date: Mon, 4 Jun 2012 18:56:18 +1000
>>>> Subject: [PATCH] SBC: LBA range check, fix some bugs in the LBA out of range check
>>>>
>>>> We can not shift the LBA << 9 and compare to the file size since this means
>>>> that for a HUGE LBA, like LBA==2^63 this will cause the 64 bit integer
>>>> to overflow and we suddenly pass all the tests and LBA sudddenly becomes LBA 0 instead.
>>>> Several targets have this bug as far as I can tell in testing.
>>>>
>>>> Instead, use LBA as is and instead shift the filesize >> 9 before the check
>>>> to avoid this overflow.
>>>>
>>>> Also, if both LBA and tranfser length are huge, LBA + TL can wrap too
>>>> so we need to check for that too and return check condition if
>>>> lba+tl < lba
>>>>
>>>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>>>> ---
>>>> usr/sbc.c | 13 +++++++------
>>>> 1 files changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/usr/sbc.c b/usr/sbc.c
>>>> index 248a547..bc9e3d6 100644
>>>> --- a/usr/sbc.c
>>>> +++ b/usr/sbc.c
>>>> @@ -297,27 +297,28 @@ static int sbc_rw(int host_no, struct scsi_cmd *cmd)
>>>> }
>>>> }
>>>>
>>>> - lba = scsi_rw_offset(cmd->scb) << cmd->dev->blk_shift;
>>>> - tl = scsi_rw_count(cmd->scb) << cmd->dev->blk_shift;
>>>> + lba = scsi_rw_offset(cmd->scb);
>>>> + tl = scsi_rw_count(cmd->scb);
>>>>
>>>> /* Verify that we are not doing i/o beyond
>>>> the end-of-lun */
>>>> if (tl) {
>>>> - if (lba + tl > lu->size) {
>>>> + if ((lba + tl < lba)
>>>
>>> How this could happen?
>>
>> For example if LBA == 0xffffffffffffffff and TL == 2, then LBA+TL == 1
>
> Ah, I see. Can you remove unnecessary () and resend an updated patch?
>
> Thanks,
Attachment:
0001-SBC-LBA-range-check-fix-some-bugs-in-the-LBA-out-of-.patch.gz
Description: GNU Zip compressed data
Attachment:
0001-SBC-LBA-range-check-fix-some-bugs-in-the-LBA-out-of-.patch
Description: Binary data
