* [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer
@ 2026-06-09 0:59 Bryam Vargas
2026-06-09 8:50 ` John Garry
0 siblings, 1 reply; 6+ messages in thread
From: Bryam Vargas @ 2026-06-09 0:59 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Mike Christie, Maurizio Lombardi, John Garry, David Disseldorp,
linux-scsi, target-devel, linux-kernel
core_scsi3_emulate_pro_register_and_move() maps the PERSISTENT RESERVE OUT
parameter list with transport_kmap_data_sg() and parses the destination
TransportID with target_parse_pr_out_transport_id(). For an iSCSI
TransportID (FORMAT CODE 01b) iscsi_parse_pr_out_transport_id() returns
the ISID as a raw pointer into that mapped buffer (the bytes following the
",i,0x" separator).
The function then unmaps the buffer with transport_kunmap_data_sg() before
dereferencing iport_ptr in strcmp(), __core_scsi3_locate_pr_reg() and
core_scsi3_alloc_registration() (the last reads 8 bytes via
get_unaligned_be64() and copies the string with snprintf()). When the
parameter list spans more than one page (PARAMETER LIST LENGTH > 4096),
transport_kmap_data_sg() uses vmap() and transport_kunmap_data_sg() does
vunmap(), so the kernel virtual address backing iport_ptr is torn down on
all architectures and every subsequent dereference is a use-after-free of
the unmapped region.
initiator_str does not have this problem because the parser strscpy()s it
into a caller-owned buffer; iport_ptr is the only output left as a borrowed
alias. core_scsi3_decode_spec_i_port() consumes the same alias safely
because it unmaps only after all uses.
Move ownership of the ISID string into the parser: after lowercasing the
ISID in-place, allocate a private copy with kzalloc(PR_REG_ISID_LEN) and
return that instead of the raw buffer pointer. kzalloc() zero-fills the
allocation to PR_REG_ISID_LEN bytes so the fixed 8-byte
get_unaligned_be64() read in __core_scsi3_do_alloc_registration() stays
in-bounds and returns deterministic results even for an ISID shorter than
8 characters (plain kstrdup() would give an allocation shorter than 8 bytes
for a malformed short ISID, turning the be64 read into a heap out-of-bounds).
Both callers now own the returned pointer and must kfree() it when done.
core_scsi3_decode_spec_i_port() frees before each inner-loop parse
iteration (so that a failed ACL match that continues the search does not
leak the previous parse's allocation) and at the error and success exit
paths. core_scsi3_emulate_pro_register_and_move() frees at both the
success return and the error label.
Fixes: 4949314c7283 ("target: Allow control CDBs with data > 1 page")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2 (review of v1 by John Garry):
- Move the ISID copy into iscsi_parse_pr_out_transport_id() so the parser
returns an owned allocation via *port_nexus_ptr; callers kfree() it.
v1's stack-buffer approach (isid_buf[] + iport_ptr = isid_buf) is gone.
- Use kzalloc(PR_REG_ISID_LEN) + strscpy_pad() rather than plain kstrdup():
__core_scsi3_do_alloc_registration() reads the ISID with a fixed 8-byte
get_unaligned_be64(isid); a malformed ISID shorter than 8 chars would
give a kstrdup allocation smaller than 8 bytes, causing a heap OOB on
that read. kzalloc zero-fills to PR_REG_ISID_LEN (16) bytes.
- core_scsi3_decode_spec_i_port(): add kfree(iport_ptr) before each
inner-loop reset (line ~1574), at out_unmap:, and before return 0.
Class / impact: CWE-416 use-after-free (use-after-vunmap) in the LIO SCSI
target. Triggerable by an authenticated iSCSI initiator that is a current
Persistent Reservation registrant on the LUN: it sends PERSISTENT RESERVE
OUT / REGISTER AND MOVE with an iSCSI (FORMAT CODE 01b) TransportID and a
PARAMETER LIST LENGTH > 4096 so the parameter list spans >1 page and is
mapped with vmap(). After transport_kunmap_data_sg() vunmap()s that region,
the retained iport_ptr is dereferenced -> kernel read of an unmapped
vmalloc address (oops / DoS; memory-safety corruption confirmed by KASAN).
Primarily a remotely-reachable authenticated denial of service.
Affected: all maintained trees -- it became a destructive dangling
dereference with 4949314c7283 (v3.3, 2012), which introduced the
multi-page vmap() path. Verified present at mainline v7.1-rc6 and
stable v6.12.92.
Reproducer (authenticated iSCSI initiator, current PR reservation holder):
1. PERSISTENT RESERVE OUT / REGISTER a key from the iSCSI nexus.
2. PERSISTENT RESERVE OUT / REGISTER AND MOVE, FORMAT CODE 01b TransportID
(IQN + ",i,0x" + 12-char ISID), RELATIVE TARGET PORT IDENTIFIER of an
existing target port, with PARAMETER LIST LENGTH = 8192 (two pages ->
vmap()/vunmap()), the inner ADDITIONAL LENGTH set so tid_len + 24 ==
data_length, the remainder zero padding.
A/B verification (CONFIG_KASAN_VMALLOC=y, kasan.fault=report, x86-64,
6.12.90; reproduced with both a 64-bit and a 32-bit initiator):
- Without this patch (8192-byte, two-page request):
BUG: KASAN: vmalloc-out-of-bounds in strcmp+0xa7/0xb0
strcmp
core_scsi3_emulate_pro_register_and_move [target_core]
? remove_vm_area
target_scsi3_emulate_pr_out [target_core]
__target_execute_cmd / iscsit_execute_cmd / iscsi_target_rx_thread
The buggy address belongs to a vmalloc virtual mapping
BUG: unable to handle page fault for address ... (PTE 0)
- Control (56/128-byte, single-page request): no report (kunmap is a
no-op on 64-bit !HIGHMEM).
- With this patch (same 8192-byte request): no report, command completes.
drivers/target/target_core_fabric_lib.c | 16 ++++++++++++++++
drivers/target/target_core_pr.c | 5 +++++
2 files changed, 21 insertions(+)
diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 87c5d26a5089..b5ad45f072bd 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -19,6 +19,7 @@
#include <linux/hex.h>
#include <linux/kernel.h>
#include <linux/string.h>
+#include <linux/slab.h>
#include <linux/ctype.h>
#include <linux/spinlock.h>
#include <linux/export.h>
@@ -367,6 +368,21 @@ static bool iscsi_parse_pr_out_transport_id(
*p = tolower(*p);
p++;
}
+ /*
+ * The loop above advanced p past the ISID; *port_nexus_ptr still
+ * holds the ISID start. Replace the borrowed buffer alias with an
+ * owned heap copy so callers can safely use the ISID past the
+ * buffer lifetime (e.g. after transport_kunmap_data_sg() in
+ * register_and_move). kzalloc() zero-fills to PR_REG_ISID_LEN
+ * bytes so the 8-byte get_unaligned_be64() read in
+ * __core_scsi3_do_alloc_registration() stays in-bounds even for
+ * an ISID shorter than 8 characters.
+ */
+ p = *port_nexus_ptr;
+ *port_nexus_ptr = kzalloc(PR_REG_ISID_LEN, GFP_KERNEL);
+ if (!*port_nexus_ptr)
+ return false;
+ strscpy_pad(*port_nexus_ptr, p, PR_REG_ISID_LEN);
} else
*port_nexus_ptr = NULL;
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 11790f2c5d80..d7340c4fbd07 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1571,6 +1571,7 @@ core_scsi3_decode_spec_i_port(
continue;
dest_rtpi = tmp_lun->lun_tpg->tpg_rtpi;
+ kfree(iport_ptr);
iport_ptr = NULL;
tid_found = target_parse_pr_out_transport_id(tmp_tpg,
ptr, &tid_len, &iport_ptr, i_str);
@@ -1808,9 +1809,11 @@ core_scsi3_decode_spec_i_port(
core_scsi3_tpg_undepend_item(dest_tpg);
}
+ kfree(iport_ptr);
return 0;
out_unmap:
transport_kunmap_data_sg(cmd);
+ kfree(iport_ptr);
out:
/*
* For the failure case, release everything from tid_dest_list
@@ -3532,10 +3535,12 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
core_scsi3_update_and_write_aptpl(cmd->se_dev, aptpl);
core_scsi3_put_pr_reg(dest_pr_reg);
+ kfree(iport_ptr);
return 0;
out:
if (buf)
transport_kunmap_data_sg(cmd);
+ kfree(iport_ptr);
if (dest_se_deve)
core_scsi3_lunacl_undepend_item(dest_se_deve);
if (dest_node_acl)
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer
2026-06-09 0:59 [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer Bryam Vargas
@ 2026-06-09 8:50 ` John Garry
2026-06-09 11:36 ` James Bottomley
0 siblings, 1 reply; 6+ messages in thread
From: John Garry @ 2026-06-09 8:50 UTC (permalink / raw)
To: Bryam Vargas, Martin K . Petersen
Cc: Mike Christie, Maurizio Lombardi, David Disseldorp, linux-scsi,
target-devel, linux-kernel
On 09/06/2026 01:59, Bryam Vargas wrote:
> core_scsi3_emulate_pro_register_and_move() maps the PERSISTENT RESERVE OUT
> parameter list with transport_kmap_data_sg() and parses the destination
> TransportID with target_parse_pr_out_transport_id(). For an iSCSI
> TransportID (FORMAT CODE 01b) iscsi_parse_pr_out_transport_id() returns
> the ISID as a raw pointer into that mapped buffer (the bytes following the
> ",i,0x" separator).
>
> The function then unmaps the buffer with transport_kunmap_data_sg() before
> dereferencing iport_ptr in strcmp(), __core_scsi3_locate_pr_reg() and
> core_scsi3_alloc_registration() (the last reads 8 bytes via
> get_unaligned_be64() and copies the string with snprintf()). When the
> parameter list spans more than one page (PARAMETER LIST LENGTH > 4096),
> transport_kmap_data_sg() uses vmap() and transport_kunmap_data_sg() does
> vunmap(), so the kernel virtual address backing iport_ptr is torn down on
> all architectures and every subsequent dereference is a use-after-free of
> the unmapped region.
>
> initiator_str does not have this problem because the parser strscpy()s it
> into a caller-owned buffer; iport_ptr is the only output left as a borrowed
> alias. core_scsi3_decode_spec_i_port() consumes the same alias safely
> because it unmaps only after all uses.
>
> Move ownership of the ISID string into the parser: after lowercasing the
> ISID in-place, allocate a private copy with kzalloc(PR_REG_ISID_LEN) and
> return that instead of the raw buffer pointer. kzalloc() zero-fills the
> allocation to PR_REG_ISID_LEN bytes so the fixed 8-byte
> get_unaligned_be64() read in __core_scsi3_do_alloc_registration() stays
> in-bounds and returns deterministic results even for an ISID shorter than
> 8 characters (plain kstrdup() would give an allocation shorter than 8 bytes
> for a malformed short ISID, turning the be64 read into a heap out-of-bounds).
>
> Both callers now own the returned pointer and must kfree() it when done.
> core_scsi3_decode_spec_i_port() frees before each inner-loop parse
> iteration (so that a failed ACL match that continues the search does not
> leak the previous parse's allocation) and at the error and success exit
> paths. core_scsi3_emulate_pro_register_and_move() frees at both the
> success return and the error label.
>
> Fixes: 4949314c7283 ("target: Allow control CDBs with data > 1 page")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---
> v2 (review of v1 by John Garry):
> - Move the ISID copy into iscsi_parse_pr_out_transport_id() so the parser
> returns an owned allocation via *port_nexus_ptr; callers kfree() it.
> v1's stack-buffer approach (isid_buf[] + iport_ptr = isid_buf) is gone.
> - Use kzalloc(PR_REG_ISID_LEN) + strscpy_pad() rather than plain kstrdup():
> __core_scsi3_do_alloc_registration() reads the ISID with a fixed 8-byte
> get_unaligned_be64(isid); a malformed ISID shorter than 8 chars would
> give a kstrdup allocation smaller than 8 bytes, causing a heap OOB on
> that read. kzalloc zero-fills to PR_REG_ISID_LEN (16) bytes.
> - core_scsi3_decode_spec_i_port(): add kfree(iport_ptr) before each
> inner-loop reset (line ~1574), at out_unmap:, and before return 0.
>
> Class / impact: CWE-416 use-after-free (use-after-vunmap) in the LIO SCSI
> target. Triggerable by an authenticated iSCSI initiator that is a current
> Persistent Reservation registrant on the LUN: it sends PERSISTENT RESERVE
> OUT / REGISTER AND MOVE with an iSCSI (FORMAT CODE 01b) TransportID and a
> PARAMETER LIST LENGTH > 4096 so the parameter list spans >1 page and is
> mapped with vmap(). After transport_kunmap_data_sg() vunmap()s that region,
> the retained iport_ptr is dereferenced -> kernel read of an unmapped
> vmalloc address (oops / DoS; memory-safety corruption confirmed by KASAN).
> Primarily a remotely-reachable authenticated denial of service.
>
> Affected: all maintained trees -- it became a destructive dangling
> dereference with 4949314c7283 (v3.3, 2012), which introduced the
> multi-page vmap() path. Verified present at mainline v7.1-rc6 and
> stable v6.12.92.
>
> Reproducer (authenticated iSCSI initiator, current PR reservation holder):
> 1. PERSISTENT RESERVE OUT / REGISTER a key from the iSCSI nexus.
> 2. PERSISTENT RESERVE OUT / REGISTER AND MOVE, FORMAT CODE 01b TransportID
> (IQN + ",i,0x" + 12-char ISID), RELATIVE TARGET PORT IDENTIFIER of an
> existing target port, with PARAMETER LIST LENGTH = 8192 (two pages ->
> vmap()/vunmap()), the inner ADDITIONAL LENGTH set so tid_len + 24 ==
> data_length, the remainder zero padding.
>
> A/B verification (CONFIG_KASAN_VMALLOC=y, kasan.fault=report, x86-64,
> 6.12.90; reproduced with both a 64-bit and a 32-bit initiator):
> - Without this patch (8192-byte, two-page request):
> BUG: KASAN: vmalloc-out-of-bounds in strcmp+0xa7/0xb0
> strcmp
> core_scsi3_emulate_pro_register_and_move [target_core]
> ? remove_vm_area
> target_scsi3_emulate_pr_out [target_core]
> __target_execute_cmd / iscsit_execute_cmd / iscsi_target_rx_thread
> The buggy address belongs to a vmalloc virtual mapping
> BUG: unable to handle page fault for address ... (PTE 0)
> - Control (56/128-byte, single-page request): no report (kunmap is a
> no-op on 64-bit !HIGHMEM).
> - With this patch (same 8192-byte request): no report, command completes.
>
> drivers/target/target_core_fabric_lib.c | 16 ++++++++++++++++
> drivers/target/target_core_pr.c | 5 +++++
> 2 files changed, 21 insertions(+)
> diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
> index 87c5d26a5089..b5ad45f072bd 100644
> --- a/drivers/target/target_core_fabric_lib.c
> +++ b/drivers/target/target_core_fabric_lib.c
> @@ -19,6 +19,7 @@
> #include <linux/hex.h>
> #include <linux/kernel.h>
> #include <linux/string.h>
> +#include <linux/slab.h>
> #include <linux/ctype.h>
> #include <linux/spinlock.h>
> #include <linux/export.h>
> @@ -367,6 +368,21 @@ static bool iscsi_parse_pr_out_transport_id(
> *p = tolower(*p);
> p++;
> }
> + /*
> + * The loop above advanced p past the ISID; *port_nexus_ptr still
> + * holds the ISID start. Replace the borrowed buffer alias with an
> + * owned heap copy so callers can safely use the ISID past the
> + * buffer lifetime (e.g. after transport_kunmap_data_sg() in
> + * register_and_move). kzalloc() zero-fills to PR_REG_ISID_LEN
> + * bytes so the 8-byte get_unaligned_be64() read in
> + * __core_scsi3_do_alloc_registration() stays in-bounds even for
> + * an ISID shorter than 8 characters.
> + */
> + p = *port_nexus_ptr;
> + *port_nexus_ptr = kzalloc(PR_REG_ISID_LEN, GFP_KERNEL);
> + if (!*port_nexus_ptr)
> + return false;
> + strscpy_pad(*port_nexus_ptr, p, PR_REG_ISID_LEN);
> } else> *port_nexus_ptr = NULL;
>
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 11790f2c5d80..d7340c4fbd07 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -1571,6 +1571,7 @@ core_scsi3_decode_spec_i_port(
> continue;
> dest_rtpi = tmp_lun->lun_tpg->tpg_rtpi;
>
> + kfree(iport_ptr);
> iport_ptr = NULL;
> tid_found = target_parse_pr_out_transport_id(tmp_tpg,
> ptr, &tid_len, &iport_ptr, i_str);
> @@ -1808,9 +1809,11 @@ core_scsi3_decode_spec_i_port(
> core_scsi3_tpg_undepend_item(dest_tpg);
> }
>
> + kfree(iport_ptr);
> return 0;
> out_unmap:
> transport_kunmap_data_sg(cmd);
> + kfree(iport_ptr);
sorry for suggesting this change, but this is worse than what you had
originally, as we have so many paths to call kfree() [which means more
bugs possible]
it's hard to make good suggestions for this target code as the functions
are so large and complex.
Is there any reason why we can't just keep the transport_kmap_data_sg()
in place for (much) longer, i.e. until at the out: label? We already
handle the the unmap properly there. 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 an difference
> out:
> /*
> * For the failure case, release everything from tid_dest_list
> @@ -3532,10 +3535,12 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
> core_scsi3_update_and_write_aptpl(cmd->se_dev, aptpl);
>
> core_scsi3_put_pr_reg(dest_pr_reg);
> + kfree(iport_ptr);
> return 0;
> out:
> if (buf)
> transport_kunmap_data_sg(cmd);
> + kfree(iport_ptr);
> if (dest_se_deve)
> core_scsi3_lunacl_undepend_item(dest_se_deve);
> if (dest_node_acl)
>
> base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer
2026-06-09 8:50 ` John Garry
@ 2026-06-09 11:36 ` James Bottomley
2026-06-09 12:14 ` John Garry
0 siblings, 1 reply; 6+ messages in thread
From: James Bottomley @ 2026-06-09 11:36 UTC (permalink / raw)
To: John Garry, Bryam Vargas, Martin K . Petersen
Cc: Mike Christie, Maurizio Lombardi, David Disseldorp, linux-scsi,
target-devel, linux-kernel
On Tue, 2026-06-09 at 09:50 +0100, John Garry wrote:
> > @@ -1808,9 +1809,11 @@ core_scsi3_decode_spec_i_port(
> > core_scsi3_tpg_undepend_item(dest_tpg);
> > }
> >
> > + kfree(iport_ptr);
> > return 0;
> > out_unmap:
> > transport_kunmap_data_sg(cmd);
> > + kfree(iport_ptr);
>
> sorry for suggesting this change, but this is worse than what you had
> originally, as we have so many paths to call kfree() [which means
> more bugs possible]
>
> it's hard to make good suggestions for this target code as the
> functions are so large and complex.
Given that it's allocated in a function called by the routine but never
actually retained by anything what about defining it as
chat *iport_ptr __free(kfree) = NULL;
? That way we don't need to care about freeing it in the error legs.
Even if the value were retained under some conditions, you just signal
retention by setting iport_ptr to NULL to prevent the kfree on exit.
Regards,
James
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer
2026-06-09 11:36 ` James Bottomley
@ 2026-06-09 12:14 ` John Garry
2026-06-09 12:27 ` James Bottomley
0 siblings, 1 reply; 6+ messages in thread
From: John Garry @ 2026-06-09 12:14 UTC (permalink / raw)
To: James Bottomley, Bryam Vargas, Martin K . Petersen
Cc: Mike Christie, Maurizio Lombardi, David Disseldorp, linux-scsi,
target-devel, linux-kernel
On 09/06/2026 12:36, James Bottomley wrote:
> On Tue, 2026-06-09 at 09:50 +0100, John Garry wrote:
>>> @@ -1808,9 +1809,11 @@ core_scsi3_decode_spec_i_port(
>>> core_scsi3_tpg_undepend_item(dest_tpg);
>>> }
>>>
>>> + kfree(iport_ptr);
>>> return 0;
>>> out_unmap:
>>> transport_kunmap_data_sg(cmd);
>>> + kfree(iport_ptr);
>>
>> sorry for suggesting this change, but this is worse than what you had
>> originally, as we have so many paths to call kfree() [which means
>> more bugs possible]
>>
>> it's hard to make good suggestions for this target code as the
>> functions are so large and complex.
>
> Given that it's allocated in a function called by the routine but never
> actually retained by anything what about defining it as
>
> chat *iport_ptr __free(kfree) = NULL;
>
> ? That way we don't need to care about freeing it in the error legs.
It's not just freed in the error legs, as
core_scsi3_decode_spec_i_port() calls target_parse_pr_out() in a loop
while (tpdl > 0) {
...
spin_lock(&dev->se_port_lock);
list_for_each_entry(tmp_lun, &dev->dev_sep_list, lun_dev_link) {
...
kfree(iport_ptr);
iport_ptr = NULL;
tid_found = target_parse_pr_out_transport_id(tmp_tpg,
ptr, &tid_len, &iport_ptr, i_str);
if (!tid_found)
continue;
....
kfree(iport_ptr);
out_unmap:
kfree(iport_ptr);
out:
...
If __free(kfree) attr can still handle this sort of flow, then ok.
I still think that it would be simpler to keep the mapping in place for
longer (to avoid all of this), if possible.
Thanks,
John
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer
2026-06-09 12:14 ` John Garry
@ 2026-06-09 12:27 ` James Bottomley
2026-06-10 4:22 ` Bryam Vargas
0 siblings, 1 reply; 6+ messages in thread
From: James Bottomley @ 2026-06-09 12:27 UTC (permalink / raw)
To: John Garry, Bryam Vargas, Martin K . Petersen
Cc: Mike Christie, Maurizio Lombardi, David Disseldorp, linux-scsi,
target-devel, linux-kernel
On Tue, 2026-06-09 at 13:14 +0100, John Garry wrote:
> On 09/06/2026 12:36, James Bottomley wrote:
> > On Tue, 2026-06-09 at 09:50 +0100, John Garry wrote:
> > > > @@ -1808,9 +1809,11 @@ core_scsi3_decode_spec_i_port(
> > > > core_scsi3_tpg_undepend_item(dest_tpg);
> > > > }
> > > >
> > > > + kfree(iport_ptr);
> > > > return 0;
> > > > out_unmap:
> > > > transport_kunmap_data_sg(cmd);
> > > > + kfree(iport_ptr);
> > >
> > > sorry for suggesting this change, but this is worse than what you
> > > had
> > > originally, as we have so many paths to call kfree() [which means
> > > more bugs possible]
> > >
> > > it's hard to make good suggestions for this target code as the
> > > functions are so large and complex.
> >
> > Given that it's allocated in a function called by the routine but
> > never
> > actually retained by anything what about defining it as
> >
> > chat *iport_ptr __free(kfree) = NULL;
> >
> > ? That way we don't need to care about freeing it in the error
> > legs.
>
> It's not just freed in the error legs, as
> core_scsi3_decode_spec_i_port() calls target_parse_pr_out() in a loop
>
> while (tpdl > 0) {
>
> ...
>
> spin_lock(&dev->se_port_lock);
> list_for_each_entry(tmp_lun, &dev->dev_sep_list,
> lun_dev_link) {
> ...
>
> kfree(iport_ptr);
> iport_ptr = NULL;
> tid_found =
> target_parse_pr_out_transport_id(tmp_tpg,
> ptr, &tid_len, &iport_ptr, i_str);
> if (!tid_found)
> continue;
>
> ....
>
> kfree(iport_ptr);
> out_unmap:
> kfree(iport_ptr);
> out:
> ...
You'd simply remove those since the pointer would be kfree'd on any
function return.
> If __free(kfree) attr can still handle this sort of flow, then ok.
It can, but there's nuance. If you do an early free (or a transfer of
ownership) you have to set the original pointer to NULL to avoid kfree
being called on it on return.
> I still think that it would be simpler to keep the mapping in place
> for longer (to avoid all of this), if possible.
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.
Regards,
James
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer
2026-06-09 12:27 ` James Bottomley
@ 2026-06-10 4:22 ` Bryam Vargas
0 siblings, 0 replies; 6+ messages in thread
From: Bryam Vargas @ 2026-06-10 4:22 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Mike Christie, Maurizio Lombardi, John Garry, James Bottomley,
David Disseldorp, linux-scsi, target-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-06-10 4:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-06-09 0:59 [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer Bryam Vargas
2026-06-09 8:50 ` John Garry
2026-06-09 11:36 ` James Bottomley
2026-06-09 12:14 ` John Garry
2026-06-09 12:27 ` James Bottomley
2026-06-10 4:22 ` Bryam Vargas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®