Re: [PATCH v2] scsi: scsi_debug: fix one-partition tape setup bounds

["Kai Mäkisara (Kolumbus)"]

> On 4. Jun 2026, at 22.29, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> On Thu, 2026-06-04 at 22:14 +0300, Kai Mäkisara (Kolumbus) wrote:
>> 
>>> On 4. Jun 2026, at 21.33, Samuel Moelius
>>> <sam.moelius@xxxxxxxxxxxxxxx> wrote:
>>> 
>>> On Thu, Jun 4, 2026 at 9:38 AM James Bottomley
>>> <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>>> 
>>>> On Wed, 2026-06-03 at 23:55 +0000, Samuel Moelius wrote:
>>>>> The tape setup path writes partition metadata one element past
>>>>> the
>>>>> allocated tape_blocks array when a one-partition configuration
>>>>> is
>>>>> selected.
>>>>> 
>>>>> That corrupts adjacent state during device initialization
>>>>> before any
>>>>> command is issued.
>>>> 
>>>> I still don't get what the actual problem is.  For a single
>>>> partition
>>>> tape I can't see where scsi_debug would actually do anything with
>>>> tape_blocks[1].  What is it that you're seeing when using
>>>> scsi_debug
>>>> that motivates this?
>>> 
>>> The bug is a kernel OOB write. I can share a PoC if desired. The
>>> PoC
>>> sends this SCSI command through /dev/sgN:
>>> 
>>> ...
>> 
>>> Then the bug: it initializes partition 1 even though there is only
>>> one
>>> partition:
>>> 
>>>    devip->tape_eop[1] = part_1_size;
>>>    devip->tape_blocks[1] = devip->tape_blocks[0] +
>>>                            devip->tape_eop[0];
>>>    devip->tape_blocks[1]->fl_size = TAPE_BLOCK_EOD_FLAG;
>>> 
>>> Because devip->tape_eop[0] == 10000, this computes:
>>> 
>>>    devip->tape_blocks[1] = devip->tape_blocks[0] + 10000
>>> 
>>> But the allocation has only 10000 elements. So this write is one
>>> element past the allocation.
>> 
>> OK. The bug is not initialization of the pointer but writing the
>> fl_size using the pointer. Good catch!
> 
> Isn't the fix actually to allocate an extra block for the EOF:
> 
> @@ -6648,7 +6648,7 @@ static int scsi_debug_sdev_configure(struct scsi_device *sdp,
>        if (sdebug_ptype == TYPE_TAPE) {
>                if (!devip->tape_blocks[0]) {
>                        devip->tape_blocks[0] =
> -                               kzalloc_objs(struct tape_block, TAPE_UNITS);
> +                               kzalloc_objs(struct tape_block, TAPE_UNITS + 1);
>                        if (!devip->tape_blocks[0])
>                                return 1;
> 
> ?
That came into my mind, too. But I considered it a workaround, not a real fix.

But after considering the other alternatives and possible problems that should be checked,
I think your suggestion is the best way. It solves tha OOB write, but does not change the code
paths.

Thanks,
Kai