[PATCH v2 0/2] s390/sclp: Misc fixes

[PATCH v2 0/2] s390/sclp: Misc fixes

[Alexander Egorenkov]

The first patch fixes 2 potential illegal memory accesses when reading
the value from a GDS subvector in SCLP event buffers sent by OCF.

The second patch fixes a race situation with sclp_unregister().

Changes since v1
----------------
- Drop redundant empty lines in sclp.c
- Make commit message more verbose for the fix in sclp.c

Alexander Egorenkov (2):
  s390/sclp_ocf: Fix computation of length of GDS values
  s390/sclp: Ensure no callback gets called after sclp_unregister()
    returns

 drivers/s390/char/sclp.c     | 19 +++++++++++++++----
 drivers/s390/char/sclp.h     |  2 ++
 drivers/s390/char/sclp_ocf.c |  4 ++--
 3 files changed, 19 insertions(+), 6 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] s390/sclp_ocf: Fix computation of length of GDS values

[Alexander Egorenkov]

There is a potential invalid read memory access while extracting
the HMC network and the CPC name from event buffers sent by OCF.

Both, the HMC network and the CPC name, are sent as a GDS subvector.
A value stored in the length field of the header of a GDS (sub)vector
includes not only the size of a GDS value but also the size of the GDS
header. Therefore, to obtain the size of the GDS value only, the size of
the GDS header must be first subtracted from the total GDS (sub)vector
length.

If the length of the HMC network or the CPC name is less than 6,
then the total length of the GDS subvector carrying it will be less than 8
(length of value plus 2 bytes for GDS subvector header). In that case
the memcpy() call in sclp_ocf_handler() will read extra 2 bytes following
the GDS subvector.

Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
---
 drivers/s390/char/sclp_ocf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/char/sclp_ocf.c b/drivers/s390/char/sclp_ocf.c
index 35f3a4a08b12..cee4bfa4a48a 100644
--- a/drivers/s390/char/sclp_ocf.c
+++ b/drivers/s390/char/sclp_ocf.c
@@ -66,13 +66,13 @@ static void sclp_ocf_handler(struct evbuf_header *evbuf)
 	/* Copy network name and cpc name. */
 	spin_lock(&sclp_ocf_lock);
 	if (netid) {
-		size = min(OCF_LENGTH_HMC_NETWORK, (size_t) netid->length);
+		size = min(OCF_LENGTH_HMC_NETWORK, (size_t) netid->length - sizeof(*netid));
 		memcpy(hmc_network, netid + 1, size);
 		EBCASC(hmc_network, size);
 		hmc_network[size] = 0;
 	}
 	if (cpc) {
-		size = min(OCF_LENGTH_CPC_NAME, (size_t) cpc->length);
+		size = min(OCF_LENGTH_CPC_NAME, (size_t) cpc->length - sizeof(*cpc));
 		memset(cpc_name, 0, OCF_LENGTH_CPC_NAME);
 		memcpy(cpc_name, cpc + 1, size);
 	}
-- 
2.53.0

[PATCH v2 2/2] s390/sclp: Ensure no callback gets called after sclp_unregister() returns

[Alexander Egorenkov]

There is a potential race condition between sclp_unregister()
and sclp_dispatch_evbufs()/sclp_dispatch_state_change(). As a result,
it is not guaranteed that the callbacks registered with sclp_register()
will not get called one more time (and no more than one) after
sclp_unregister() returns.

Example of a race situation
===========================

         CPU A (SCLP)                 CPU B (driver using SCLP API)

--> sclp_dispatch_evbufs()
 --> spin_unlock(&sclp_lock)

                                    --> sclp_unregister()
				     --> spin_lock(&sclp_lock)
				     --> remove struct sclp_register from list
				     --> spin_unlock(&sclp_lock)
                                    <-- sclp_unregister()

                                    --> free struct sclp_register

 --> receiver_fn()
  --> illegal memory access

 --> spin_lock(&sclp_lock)
<-- sclp_dispatch_evbufs()

To guarantee that this race situation never occurs, it must be ensured
that sclp_unregister() waits for the last callback invocation by
sclp_dispatch_evbufs()/sclp_dispatch_state_change() to return.
The basic idea is to use a single completion per struct sclp_register
to block in sclp_unregister() until the last callback completes.
It is sufficient to use only one completion per struct sclp_register
because the callbacks state_change_fn() and receiver_fn() never get called
in parallel.

Initially, the completion is marked as done in sclp_register() and every
time when sclp_dispatch_evbufs()/sclp_dispatch_state_change() calls
one of the callbacks, the completion is marked as incomplete before
its invocation and as done afterwards. The call to wait_for_completion()
in sclp_unregister() shall block until the last invocation of the callback
finishes. And if no callback is pending or being invoked by
sclp_dispatch_evbufs()/sclp_dispatch_state_change() then the completion
is always marked as done and sclp_unregister() shall never block in
wait_for_completion() and return quickly.

To mark the completion as incomplete, reinit_completion() is needed and it
must be guaranteed that no racy wait_for_completion() calls are going on in
parallel. This is achieved by calling reinit_completion() with the SCLP
spinlock held. The following situations must be considered:
1. No callback invocation is pending or in progress when
   wait_for_completion() is called from sclp_unregister(). This is
   a regular situation occurring in most of the cases. wait_for_completion()
   will return quickly because the completion is marked as done. When
   wait_for_completion() gets called, the callbacks no longer exist in
   the list sclp_reg_list, and, therefore,
   sclp_dispatch_evbufs()/sclp_dispatch_state_change() would not be able
   to obtain one of them and invoke reinit_completion() while
   a wait_for_completion() call is going on in parallel.
2. One of the callbacks is pending or in progress when
   sclp_unregister() gets called. This is a rare situation.
   The SCLP spinlock is either being held by
   sclp_dispatch_evbufs()/sclp_dispatch_state_change() or not. In the former
   case, sclp_unregister() would block in the SCLP spinlock trying to
   remove the callbacks from the list and not enter wait_for_completion().
   The SCLP spinlock is released after a reinit_complete() call, therefore,
   it will not race with a wait_for_complete() call in sclp_unregister().
   If the SCLP spinlock is not held by
   sclp_dispatch_evbufs()/sclp_dispatch_state_change(), then
   a reinit_complete() call must have been completed already and
   it will not race with a wait_for_complete() call in sclp_unregister().

With this change sclp_unregister() may no longer be invoked
from atomic context or registered callbacks. But this should not be a
problem because this is no regular use case and no driver using
sclp_register()/sclp_unregister() requires this at the moment and
likely should not require it in the future.

Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
---
 drivers/s390/char/sclp.c | 19 +++++++++++++++----
 drivers/s390/char/sclp.h |  2 ++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c
index 98e334724a62..8954f4260401 100644
--- a/drivers/s390/char/sclp.c
+++ b/drivers/s390/char/sclp.c
@@ -565,9 +565,11 @@ sclp_dispatch_evbufs(struct sccb_header *sccb)
 				 evbuf, !reg);
 
 		if (reg && reg->receiver_fn) {
+			reinit_completion(&reg->done);
 			spin_unlock_irqrestore(&sclp_lock, flags);
 			reg->receiver_fn(evbuf);
 			spin_lock_irqsave(&sclp_lock, flags);
+			complete(&reg->done);
 		} else if (reg == NULL)
 			rc = -EOPNOTSUPP;
 	}
@@ -773,8 +775,8 @@ sclp_dispatch_state_change(void)
 	sccb_mask_t receive_mask;
 	sccb_mask_t send_mask;
 
+	spin_lock_irqsave(&sclp_lock, flags);
 	do {
-		spin_lock_irqsave(&sclp_lock, flags);
 		reg = NULL;
 		list_for_each(l, &sclp_reg_list) {
 			reg = list_entry(l, struct sclp_register, list);
@@ -788,15 +790,18 @@ sclp_dispatch_state_change(void)
 			} else
 				reg = NULL;
 		}
-		spin_unlock_irqrestore(&sclp_lock, flags);
 		if (reg && reg->state_change_fn) {
+			reinit_completion(&reg->done);
+			spin_unlock_irqrestore(&sclp_lock, flags);
 			/* STCG: State-change callback (b=callback) */
 			sclp_trace(2, "STCG", 0, (u64)reg->state_change_fn,
 				   false);
-
 			reg->state_change_fn(reg);
+			spin_lock_irqsave(&sclp_lock, flags);
+			complete(&reg->done);
 		}
 	} while (reg);
+	spin_unlock_irqrestore(&sclp_lock, flags);
 }
 
 struct sclp_statechangebuf {
@@ -885,6 +890,8 @@ sclp_register(struct sclp_register *reg)
 	/* Trigger initial state change callback */
 	reg->sclp_receive_mask = 0;
 	reg->sclp_send_mask = 0;
+	init_completion(&reg->done);
+	complete(&reg->done);
 	list_add(&reg->list, &sclp_reg_list);
 	spin_unlock_irqrestore(&sclp_lock, flags);
 	rc = sclp_init_mask(1);
@@ -898,7 +905,10 @@ sclp_register(struct sclp_register *reg)
 
 EXPORT_SYMBOL(sclp_register);
 
-/* Unregister event listener. */
+/* Unregister event listener.
+ * It can sleep and, therefore, may not get called from atomic context.
+ * And neither it can get called from a receive_fn() callback because
+ * struct sclp_register must remain valid after the call. */
 void
 sclp_unregister(struct sclp_register *reg)
 {
@@ -911,6 +921,7 @@ sclp_unregister(struct sclp_register *reg)
 	list_del(&reg->list);
 	spin_unlock_irqrestore(&sclp_lock, flags);
 	sclp_init_mask(1);
+	wait_for_completion(&reg->done);
 }
 
 EXPORT_SYMBOL(sclp_unregister);
diff --git a/drivers/s390/char/sclp.h b/drivers/s390/char/sclp.h
index b31a680e0871..b04f6287ccfc 100644
--- a/drivers/s390/char/sclp.h
+++ b/drivers/s390/char/sclp.h
@@ -11,6 +11,7 @@
 
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/completion.h>
 #include <asm/asm-extable.h>
 #include <asm/machine.h>
 #include <asm/sclp.h>
@@ -275,6 +276,7 @@ struct sclp_register {
 	void (*state_change_fn)(struct sclp_register *);
 	/* called for events in cp_receive_mask/sclp_receive_mask */
 	void (*receiver_fn)(struct evbuf_header *);
+	struct completion done;
 };
 
 /* externals from sclp.c */
-- 
2.53.0

Re: [PATCH v2 1/2] s390/sclp_ocf: Fix computation of length of GDS values

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Integer underflow when computing GDS subvector payload length leads to an 8-byte out-of-bounds read.

Pre-existing issues:
- [High] Parsing GDS vectors and subvectors without checking for zero length leads to an infinite loop.
- [High] Missing struct size and buffer bounds checking in GDS parsing loops allows out-of-bounds memory reads.
--

--- Patch [1]: [PATCH v2 1/2] s390/sclp_ocf: Fix computation of length of GDS values ---
commit 89f746e9734f7be9cdc7f0e26a7a861dc8653aae
Author: Alexander Egorenkov <egorenar@linux.ibm.com>

s390/sclp_ocf: Fix computation of length of GDS values
    
This commit attempts to fix a potential invalid read memory access while
extracting the HMC network and the CPC name from event buffers sent by OCF.
It subtracts the GDS header size from the total length to calculate the exact
size of the GDS value.

> diff --git a/drivers/s390/char/sclp_ocf.c b/drivers/s390/char/sclp_ocf.c
> index 35f3a4a08b123..cee4bfa4a48aa 100644
> --- a/drivers/s390/char/sclp_ocf.c
> +++ b/drivers/s390/char/sclp_ocf.c
> @@ -66,13 +66,13 @@ static void sclp_ocf_handler(struct evbuf_header *evbuf)
>  	/* Copy network name and cpc name. */
>  	spin_lock(&sclp_ocf_lock);
>  	if (netid) {
> -		size = min(OCF_LENGTH_HMC_NETWORK, (size_t) netid->length);
> +		size = min(OCF_LENGTH_HMC_NETWORK, (size_t) netid->length - sizeof(*netid));

[Severity: High]
Does this subtraction safely handle small length values?

If the hardware provides a malformed SCLP event buffer where netid->length is
0 or 1, subtracting sizeof(*netid) (which is 2) will underflow. Since the cast
to size_t makes the result unsigned, this underflows to a large positive value
like SIZE_MAX.

The min() function will then evaluate to OCF_LENGTH_HMC_NETWORK (8), leading
memcpy() to read out-of-bounds bytes from the event buffer.

>  		memcpy(hmc_network, netid + 1, size);
>  		EBCASC(hmc_network, size);
>  		hmc_network[size] = 0;
>  	}
>  	if (cpc) {
> -		size = min(OCF_LENGTH_CPC_NAME, (size_t) cpc->length);
> +		size = min(OCF_LENGTH_CPC_NAME, (size_t) cpc->length - sizeof(*cpc));

[Severity: High]
Could this code also underflow cpc->length?

Similar to the netid block above, if cpc->length is 0 or 1, subtracting
sizeof(*cpc) will underflow to a large unsigned value, bypassing the min()
clamp and causing memcpy() to read 8 bytes out-of-bounds.

>  		memset(cpc_name, 0, OCF_LENGTH_CPC_NAME);
>  		memcpy(cpc_name, cpc + 1, size);
>  	}

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the GDS parsing helpers
used earlier in this path:

drivers/s390/char/sclp.h:sclp_find_gds_subvector() {
    ...
	for (sv = start; (void *) sv < end; sv = (void *) sv + sv->length)
		if (sv->key == key)
			return sv;
    ...
}

Does this parsing loop prevent infinite loops on malformed buffers?

If the buffer contains a subvector where sv->length is 0, the pointer sv will
not advance, resulting in an infinite loop if the key doesn't match.

Additionally, does this condition ensure the full struct and its payload reside
within the buffer? The loop condition ((void *) sv < end) allows entry even if
there is only 1 byte left in the buffer. This means reading sv->length and
sv->key could result in an out-of-bounds memory read. It also doesn't verify
if sv + sv->length exceeds the end boundary.

The same concerns apply to sclp_find_gds_vector().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804074511.236153-1-egorenar@linux.ibm.com?part=1

Re: [PATCH v2 2/2] s390/sclp: Ensure no callback gets called after sclp_unregister() returns

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing synchronization in sclp_register() error path leads to Use-After-Free.
- [Medium] Concurrent execution of state_change_fn and receiver_fn corrupts the completion mechanism.
--

--- Patch [2]: [PATCH v2 2/2] s390/sclp: Ensure no callback gets called after sclp_unregister() returns ---
commit 56ff5dbe692c5051e8907f572849ceb9402fdda1
Author: Alexander Egorenkov <egorenar@linux.ibm.com>

s390/sclp: Ensure no callback gets called after sclp_unregister() returns

This commit adds a completion mechanism to prevent callbacks from being 
executed after a driver unregisters from the SCLP subsystem. It blocks 
sclp_unregister() until any inflight state change or receive callbacks 
have finished executing.

> It is sufficient to use only one completion per struct sclp_register
> because the callbacks state_change_fn() and receiver_fn() never get called
> in parallel.

[Severity: Medium]
Is this assumption accurate?

Looking at sclp_dispatch_state_change() and sclp_dispatch_evbufs(), both 
functions drop sclp_lock before invoking their respective callbacks. If a 
driver registers both callbacks (such as sclp_input_event in sclp_tty.c), 
could both dispatchers execute concurrently?

If they run in parallel, it seems they would both call reinit_completion() 
and complete() on the same reg->done completion, potentially corrupting it 
and causing a premature wakeup in sclp_unregister().

> diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c
> index 98e334724a627..8954f4260401b 100644
> --- a/drivers/s390/char/sclp.c
> +++ b/drivers/s390/char/sclp.c

[ ... ]

> @@ -885,6 +890,8 @@ sclp_register(struct sclp_register *reg)
>  	/* Trigger initial state change callback */
>  	reg->sclp_receive_mask = 0;
>  	reg->sclp_send_mask = 0;
> +	init_completion(&reg->done);
> +	complete(&reg->done);
>  	list_add(&reg->list, &sclp_reg_list);
>  	spin_unlock_irqrestore(&sclp_lock, flags);
>  	rc = sclp_init_mask(1);

[Severity: High]
If sclp_init_mask(1) fails here and returns an error, the listener is removed
from sclp_reg_list and sclp_register() returns the error to the caller.

Since the listener was briefly in the active list, could a hardware interrupt
have started executing the callback concurrently? 

If so, does this error path also need to call wait_for_completion(&reg->done)
before returning? Otherwise, it looks like a concurrently executing callback
could call complete(&reg->done) on a freed structure if the caller frees the
listener after the registration failure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804074511.236153-1-egorenar@linux.ibm.com?part=2