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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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@trailofbits.com> wrote:
> > 
> > On Thu, Jun 4, 2026 at 9:38 AM James Bottomley
> > <James.Bottomley@hansenpartnership.com> 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;

?

Regards,

James