[stgt] [PATCH] Another patch to improve the LBA out of range even further

[ronnie sahlberg]

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 at lab.ntt.co.jp> wrote:
> On Sun, 17 Jun 2012 09:39:00 +1000
> ronnie sahlberg <ronniesahlberg at gmail.com> wrote:
>
>> On Sun, Jun 17, 2012 at 8:53 AM, FUJITA Tomonori
>> <fujita.tomonori at lab.ntt.co.jp> wrote:
>>> On Mon, 4 Jun 2012 19:10:12 +1000
>>> ronnie sahlberg <ronniesahlberg at gmail.com> wrote:
>>>
>>>> From efc1f1720ad9a00e69f7935a001df906e609afe5 Mon Sep 17 00:00:00 2001
>>>> From: Ronnie Sahlberg <ronniesahlberg at gmail.com>
>>>> 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 at gmail.com>
>>>> ---
>>>>  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,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SBC-LBA-range-check-fix-some-bugs-in-the-LBA-out-of-.patch.gz
Type: application/x-gzip
Size: 969 bytes
Desc: not available
URL: <http://lists.wpkg.org/pipermail/stgt/attachments/20120617/fd9e88d5/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SBC-LBA-range-check-fix-some-bugs-in-the-LBA-out-of-.patch
Type: application/octet-stream
Size: 1949 bytes
Desc: not available
URL: <http://lists.wpkg.org/pipermail/stgt/attachments/20120617/fd9e88d5/attachment-0002.obj>