Re: [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer

[Bryam Vargas <hexlabsecurity@proton.me>]

On 2026-06-09, James Bottomley wrote:
> I was just looking at the multiple error legs in the patched code and
> thinking it's an ideal candidate for a __free() attribute.  If there's
> a way of rearranging the code to keep it longer that you think is more
> readable, by all means do that instead.

I'll keep the mapping in place, which turns out to be the simplest of the
three options: it needs no ownership transfer at all, so neither kfree()
nor __free() comes into it.

The only reason v2 had an iport_ptr to free was that v2 made the parser
allocate.  If the PR-OUT parameter list instead stays mapped, iport_ptr
can remain what it has always been -- a borrowed alias into that buffer --
and v3 is just:

  - drop the early transport_kunmap_data_sg() / buf = NULL after the parse;
  - unmap once on the success path, right before "return 0";
  - the existing "if (buf) transport_kunmap_data_sg(cmd)" at out: now
    covers every error leg, because buf is no longer cleared early.

That is -2/+1 lines (plus a comment) in
core_scsi3_emulate_pro_register_and_move() only.
target_core_fabric_lib.c is untouched, and core_scsi3_decode_spec_i_port()
needs no change -- it already uses iport_ptr before its own unmap, so the
loop/free question there does not arise.

On 2026-06-09, John Garry wrote:
> I do notice that there would be regions which we keep spinlocks held
> when this mapping is in place, but I am not sure if that makes a
> difference

It does not.  Holding an already-established kmap/vmap mapping across a
spinlock is fine; what is not allowed is calling kmap()/kunmap() (or
vmap()/vunmap(), which may sleep) while atomic.  In this version both the
map and the unmap happen outside any lock -- the map before
dev_reservation_lock is taken, the unmap after it is dropped (and at out:
after all locks are released).  The only thing done under
dev_reservation_lock is __core_scsi3_locate_pr_reg() reading the ISID
through the mapping, which is an ordinary load.

I re-ran the KASAN A/B for this form on the same CONFIG_KASAN_VMALLOC rig
(6.12.90, 64-bit and 32-bit initiator): unpatched, an 8192-byte (two-page)
REGISTER AND MOVE reports vmalloc-out-of-bounds in strcmp() and the rx
kthread dies; with the patch the same request completes with no report.

v3 follows as a separate posting.

Thanks,
Bryam