[PATCH v5 0/4] s390/sclp: Misc fixes

[PATCH v5 0/4] s390/sclp: Misc fixes

[Alexander Egorenkov]

This series consists of several fixes for the s390 SCLP driver.

* The first patch introduces the macro sclp_gds_for_each() to safely iterate over
  GDS {sub}vectors and serves to improve error handling in sclp_find_gds_{sub}vector()
  to prevent out-of-range memory read and potential infinite loops
  when a malformed event buffer is received from SCLP.
* The second patch reuses the macro sclp_gds_for_each() in SCLP TTY introduced in the first patch
  to replace manual and error-prone iteration over entries of a GDS {sub}vector to fix
  the same issues addressed in the first patch.
* The third patch fixes 2 potential illegal memory accesses when reading
  the value from a GDS subvector in SCLP event buffers sent by OCF.
* The fourth patch fixes race situations with in-flight callbacks and sclp_unregister() calls.

Changes since v4
----------------
- Drop patch "s390/sclp: Drop volatile type class from SCLP state variables"
  - There are several doubts to it being correct in all situations
- Introduce the macro sclp_gds_for_each()
  - Reusable and safe iteration over GDS {sub}vectors
  - Make use of sclp_gds_for_each() in SCLP TTY
- Rework the patch "s390/sclp: Ensure no callback gets called after sclp_{un}register() returns"
  - Remove waiting for SCLP mask and reading states to become idle from sclp_register() on sclp_init_mask() failure
    - First, it is incorrect to sleep in sclp_register() which is called from atomic context
      - sclp_console_init() -> sclp_rw_init() -> sclp_register()
      - sclp_vt220_con_init() -> __sclp_vt220_init() -> sclp_register()
    - Second, it is redundant because no race situation can occur if sclp_init_mask() fails because
      in that case no events can be received from SCLP due to SCLP receive event mask update performed
      in sclp_init_mask() having failed
  - Add might_sleep() to sclp_unregister() to indicate that the function could potentially sleep
  - Adjust coding style of function sclp_unregister()
- Add "Fixes" tag where necessary

Changes since v3
----------------
- Rework the patch "s390/sclp: Ensure no callback gets called after sclp_{un}register() returns"
  - Shorten and reword the commit description
  - Replace wake_up_all_locked() with wake_up_all()
  - Replace sclp_init_state with sclp_mask_state in wait queue condition
  - Call wake_up_all() unconditionally
  - Add call to wake_up_call() in sclp_init_mask() after updating sclp_mask_state 

Changes since v2
----------------
- Add 2 new patches:
  - s390/sclp: Drop volatile type class from SCLP state variables
  - s390/sclp: Improve robustness of sclp_find_gds_{sub}vector()
- Rework the patch "s390/sclp: Ensure no callback gets called after sclp_unregister() returns"
  to implement Peter Oberparleiter's suggestion with a global wait queue and checking
  the state variables as its condition. It turns out the implementation with a single completion
  per struct sclp_register is inadequate because theoretically the callback state_change_fn() and receive_fn()
  could get invoked in parallel, however unlikely. Furthermore, the same race situation might happen
  with sclp_register() too. 

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

Alexander Egorenkov (4):
  s390/sclp: Introduce macro sclp_gds_for_each()
  s390/sclp_tty: Make use of sclp_gds_for_each()
  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     | 24 ++++++++++++++++++------
 drivers/s390/char/sclp.h     | 24 ++++++++++++++++--------
 drivers/s390/char/sclp_ocf.c |  4 ++--
 drivers/s390/char/sclp_tty.c | 30 +++++++++++++++---------------
 4 files changed, 51 insertions(+), 31 deletions(-)

-- 
2.53.0

[PATCH v5 1/4] s390/sclp: Introduce macro sclp_gds_for_each()

[Alexander Egorenkov]

sclp_find_gds_{sub}vector() does not deal well with malformed event buffers
consisting of GDS {sub}vectors. This can result in an infinite loop or
an out-of-bounds memory read. Therefore, abort with NULL if
* the next GDS header would exceed the given end boundary
* the length in a GDS header contains an invalid value.
  A valid length value in a GDS header should be at least as large
  as the size of the corresponding GDS header (2 or 4 bytes)
  but also not lead to exceeding the given end boundary.

Use the new macro in sclp_find_gds_{sub}vector() to iterates over entries
of a GDS {sub}vector in a safe manner bailing out on the first invalid
entry.

Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Suggested-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Fixes: 30c2df51173e ("[S390] sclp: event buffer dissection")
---
 drivers/s390/char/sclp.h | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/char/sclp.h b/drivers/s390/char/sclp.h
index b31a680e0871..d22003b769f1 100644
--- a/drivers/s390/char/sclp.h
+++ b/drivers/s390/char/sclp.h
@@ -360,25 +360,33 @@ sclp_ascebc_str(char *str, int nr)
 	(machine_is_vm()) ? ASCEBC(str, nr) : ASCEBC_500(str, nr);
 }
 
-static inline struct gds_vector *
-sclp_find_gds_vector(void *start, void *end, u16 id)
+/* Loop over all GDS {sub}vectors in a safe manner. */
+#define sclp_gds_for_each(v, n, start, end)	\
+	for ((n) = (end) - (start), (v) = (start);	\
+	     (n) >= sizeof(*(v)) && (v)->length >= sizeof(*(v)) && (v)->length <= (n);	\
+	     (n) -= (v)->length, (v) = (void*)(v) + (v)->length)
+
+static inline struct gds_vector *sclp_find_gds_vector(void *start,
+						      void *end, u16 id)
 {
 	struct gds_vector *v;
-
-	for (v = start; (void *) v < end; v = (void *) v + v->length)
+	int n;
+	sclp_gds_for_each(v, n, start, end) {
 		if (v->gds_id == id)
 			return v;
+	}
 	return NULL;
 }
 
-static inline struct gds_subvector *
-sclp_find_gds_subvector(void *start, void *end, u8 key)
+static inline struct gds_subvector *sclp_find_gds_subvector(void *start,
+							   void *end, u8 key)
 {
 	struct gds_subvector *sv;
-
-	for (sv = start; (void *) sv < end; sv = (void *) sv + sv->length)
+	int n;
+	sclp_gds_for_each(sv, n, start, end) {
 		if (sv->key == key)
 			return sv;
+	}
 	return NULL;
 }
 
-- 
2.53.0

[PATCH v5 2/4] s390/sclp_tty: Make use of sclp_gds_for_each()

[Alexander Egorenkov]

Replace manual and error-prone iteration over entries of a GDS {sub}vector
with sclp_gds_for_each() which deals with erroneous entries in GDS
{sub}vectors more safely.

Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Suggested-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Fixes: 30c2df51173e ("[S390] sclp: event buffer dissection")
---
 drivers/s390/char/sclp_tty.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/s390/char/sclp_tty.c b/drivers/s390/char/sclp_tty.c
index 0a92d08830e7..4353890bbf8a 100644
--- a/drivers/s390/char/sclp_tty.c
+++ b/drivers/s390/char/sclp_tty.c
@@ -418,38 +418,38 @@ static void sclp_get_input(struct gds_subvector *sv)
 
 static inline void sclp_eval_selfdeftextmsg(struct gds_subvector *sv)
 {
-	void *end;
-
-	end = (void *) sv + sv->length;
-	for (sv = sv + 1; (void *) sv < end; sv = (void *) sv + sv->length)
+	void *start = sv + 1;
+	void *end = (void *) sv + sv->length;
+	int n;
+	sclp_gds_for_each(sv, n, start, end) {
 		if (sv->key == 0x30)
 			sclp_get_input(sv);
+	}
 }
 
 static inline void sclp_eval_textcmd(struct gds_vector *v)
 {
+	void *start = v + 1;
+	void *end = (void *) v + v->length;
 	struct gds_subvector *sv;
-	void *end;
-
-	end = (void *) v + v->length;
-	for (sv = (struct gds_subvector *) (v + 1);
-	     (void *) sv < end; sv = (void *) sv + sv->length)
+	int n;
+	sclp_gds_for_each(sv, n, start, end) {
 		if (sv->key == GDS_KEY_SELFDEFTEXTMSG)
 			sclp_eval_selfdeftextmsg(sv);
-
+	}
 }
 
 static inline void sclp_eval_cpmsu(struct gds_vector *v)
 {
-	void *end;
-
-	end = (void *) v + v->length;
-	for (v = v + 1; (void *) v < end; v = (void *) v + v->length)
+	void *start = v + 1;
+	void *end = (void *) v + v->length;
+	int n;
+	sclp_gds_for_each(v, n, start, end) {
 		if (v->gds_id == GDS_ID_TEXTCMD)
 			sclp_eval_textcmd(v);
+	}
 }
 
-
 static inline void sclp_eval_mdsmu(struct gds_vector *v)
 {
 	v = sclp_find_gds_vector(v + 1, (void *) v + v->length, GDS_ID_CPMSU);
-- 
2.53.0

[PATCH v5 3/4] 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>
Fixes: 7eb9d5bec552 ("[S390] get CPC image name")
---
 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 v5 4/4] 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.

The basic idea is to use a single global wait queue which is woken up
when no SCLP WRITE_EVENT_MASK and no SCLP READ_EVENT_DATA request is
outstanding. If both are true then it is guaranteed that no sclp_register
callback could be in-flight. Once the struct sclp_register given to
sclp_unregister() is removed from the list sclp_reg_list, no further
callbacks can be scheduled for the given struct sclp_register and it only
remains to wait until the SCLP driver becomes idle.

With this change sclp_unregister() may no longer be invoked from atomic
context or registered callbacks. But this represents no problem because
this is no regular use case and no driver using 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>
Suggested-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
---
 drivers/s390/char/sclp.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c
index 98e334724a62..c064234314bf 100644
--- a/drivers/s390/char/sclp.c
+++ b/drivers/s390/char/sclp.c
@@ -20,6 +20,7 @@
 #include <linux/jiffies.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
+#include <linux/wait.h>
 #include <asm/types.h>
 #include <asm/irq.h>
 #include <asm/debug.h>
@@ -67,6 +68,8 @@ static struct sclp_req sclp_init_req;
 static void *sclp_read_sccb;
 static struct init_sccb *sclp_init_sccb;
 
+static DECLARE_WAIT_QUEUE_HEAD(sclp_state_wq);
+
 /* Number of console pages to allocate, used by sclp_con.c and sclp_vt220.c */
 int sclp_console_pages = SCLP_CONSOLE_PAGES;
 /* Flag to indicate if buffer pages are dropped on buffer full condition */
@@ -588,6 +591,7 @@ sclp_read_cb(struct sclp_req *req, void *data)
 		sclp_dispatch_evbufs(sccb);
 	spin_lock_irqsave(&sclp_lock, flags);
 	sclp_reading_state = sclp_reading_state_idle;
+	wake_up_all(&sclp_state_wq);
 	spin_unlock_irqrestore(&sclp_lock, flags);
 }
 
@@ -898,19 +902,26 @@ sclp_register(struct sclp_register *reg)
 
 EXPORT_SYMBOL(sclp_register);
 
-/* Unregister event listener. */
-void
-sclp_unregister(struct sclp_register *reg)
+/* Unregister event listener.
+ * This function may sleep. Do not call it from atomic context or
+ * sclp_register.receive_fn(). */
+void sclp_unregister(struct sclp_register *reg)
 {
-	unsigned long flags;
+	might_sleep();
 
 	/* UREG: Event listener unregistered (b=caller) */
 	sclp_trace_register(2, "UREG", 0, _RET_IP_, reg);
 
-	spin_lock_irqsave(&sclp_lock, flags);
+	spin_lock_irq(&sclp_lock);
 	list_del(&reg->list);
-	spin_unlock_irqrestore(&sclp_lock, flags);
+	spin_unlock_irq(&sclp_lock);
 	sclp_init_mask(1);
+	spin_lock_irq(&sclp_lock);
+	wait_event_lock_irq(sclp_state_wq,
+			    (sclp_mask_state == sclp_mask_state_idle) &&
+			    (sclp_reading_state == sclp_reading_state_idle),
+			    sclp_lock);
+	spin_unlock_irq(&sclp_lock);
 }
 
 EXPORT_SYMBOL(sclp_unregister);
@@ -1036,6 +1047,7 @@ sclp_init_mask(int calculate)
 		}
 	}
 	sclp_mask_state = sclp_mask_state_idle;
+	wake_up_all(&sclp_state_wq);
 	spin_unlock_irqrestore(&sclp_lock, flags);
 	return rc;
 }
-- 
2.53.0

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

[sashiko-bot]

> 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.
> 
> The basic idea is to use a single global wait queue which is woken up
> when no SCLP WRITE_EVENT_MASK and no SCLP READ_EVENT_DATA request is
> outstanding. If both are true then it is guaranteed that no sclp_register
> callback could be in-flight. Once the struct sclp_register given to
> sclp_unregister() is removed from the list sclp_reg_list, no further
> callbacks can be scheduled for the given struct sclp_register and it only
> remains to wait until the SCLP driver becomes idle.
> 
> With this change sclp_unregister() may no longer be invoked from atomic
> context or registered callbacks. But this represents no problem because
> this is no regular use case and no driver using 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>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v5 1/4] s390/sclp: Introduce macro sclp_gds_for_each()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sclp: out-of-bounds read in sclp_gds_for_each() due to integer promotion
--

commit f292c3c2b28abf9088d99e5a8af81f738a00c9ee
Author: Alexander Egorenkov <egorenar@linux.ibm.com>

s390/sclp: Introduce macro sclp_gds_for_each()

This commit introduces a macro to iterate safely over GDS {sub}vectors. It
adds bounds checks to avoid infinite loops and out-of-bounds memory accesses
when parsing malformed event buffers.

> diff --git a/drivers/s390/char/sclp.h b/drivers/s390/char/sclp.h
> index b31a680e08714..d22003b769f14 100644
> --- a/drivers/s390/char/sclp.h
> +++ b/drivers/s390/char/sclp.h
> @@ -360,25 +360,33 @@ sclp_ascebc_str(char *str, int nr)
>  	(machine_is_vm()) ? ASCEBC(str, nr) : ASCEBC_500(str, nr);
>  }
>  
> -static inline struct gds_vector *
> -sclp_find_gds_vector(void *start, void *end, u16 id)
> +/* Loop over all GDS {sub}vectors in a safe manner. */
> +#define sclp_gds_for_each(v, n, start, end)	\
> +	for ((n) = (end) - (start), (v) = (start);	\
> +	     (n) >= sizeof(*(v)) && (v)->length >= sizeof(*(v)) && (v)->length <= (n);	\

[Severity: High]
Does this code introduce an out-of-bounds read due to integer promotion?

If a malformed SCLP event buffer is provided by the hardware with an
evbuf->length smaller than the header size, start will be greater than end.
This happens in callers like sclp_ocf_handler() and sclp_tty_receiver():

drivers/s390/char/sclp_ocf.c:sclp_ocf_handler() {
    ...
    v = sclp_find_gds_vector(evbuf + 1, (void *) evbuf + evbuf->length,
                             0x9f00);
    ...
}

In sclp_gds_for_each(), the loop limit n is computed as (end) - (start).
Because callers define n as an int, if start is greater than end, n will be a
negative integer.

The bounds condition (n) >= sizeof(*(v)) compares the signed int n with the
unsigned size_t returned by sizeof. Due to C integer promotion rules, n is
implicitly promoted to an unsigned type, wrapping the negative value to a very
large positive number.

Consequently, the bounds check evaluates to true, and the loop mistakenly
executes (v)->length >= sizeof(*(v)), reading from the out-of-bounds start
pointer.

The original logic ((void *) v < end) handled start > end safely by
immediately skipping the loop.

> +	     (n) -= (v)->length, (v) = (void*)(v) + (v)->length)
> +
> +static inline struct gds_vector *sclp_find_gds_vector(void *start,
> +						      void *end, u16 id)
>  {
>  	struct gds_vector *v;
> -
> -	for (v = start; (void *) v < end; v = (void *) v + v->length)
> +	int n;
> +	sclp_gds_for_each(v, n, start, end) {
>  		if (v->gds_id == id)
>  			return v;
> +	}
>  	return NULL;
>  }

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

Re: [PATCH v5 3/4] s390/sclp_ocf: Fix computation of length of GDS values

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v5 2/4] s390/sclp_tty: Make use of sclp_gds_for_each()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] s390/sclp: out-of-bounds read in sclp_gds_for_each() due to integer promotion
--

commit b02b1435e7ea0c6d35613a83d6a0bafa8d59fb95
Author: Alexander Egorenkov <egorenar@linux.ibm.com>

s390/sclp_tty: Make use of sclp_gds_for_each()

This commit replaces manual iterations over gds subvector entries with the
sclp_gds_for_each() macro. The stated goal is to handle erroneous entries
in gds vectors and subvectors more safely.

> diff --git a/drivers/s390/char/sclp_tty.c b/drivers/s390/char/sclp_tty.c
> index 0a92d08830e7c..4353890bbf8af 100644
> --- a/drivers/s390/char/sclp_tty.c
> +++ b/drivers/s390/char/sclp_tty.c
> @@ -418,38 +418,38 @@ static void sclp_get_input(struct gds_subvector *sv)
>  
>  static inline void sclp_eval_selfdeftextmsg(struct gds_subvector *sv)
>  {
> -	void *end;
> -
> -	end = (void *) sv + sv->length;
> -	for (sv = sv + 1; (void *) sv < end; sv = (void *) sv + sv->length)
> +	void *start = sv + 1;
> +	void *end = (void *) sv + sv->length;
> +	int n;
> +	sclp_gds_for_each(sv, n, start, end) {

[Severity: High]
Does this code introduce an out-of-bounds read regression if the hardware
provides a malformed sccb buffer with a length smaller than the header size?

If sv->length is smaller than sizeof(*sv), start will be greater than end.
In the sclp_gds_for_each() macro defined in drivers/s390/char/sclp.h, n is
defined as an int and evaluated as n = (end) - (start), which becomes a
negative value:

drivers/s390/char/sclp.h:
#define sclp_gds_for_each(v, n, start, end) \
	for ((n) = (end) - (start), (v) = (start); \
	     (n) >= sizeof(*(v)) && (v)->length >= sizeof(*(v)) && (v)->length <= (n); \
	     (n) -= (v)->length, (v) = (void*)(v) + (v)->length)

Because sizeof returns an unsigned size_t, standard C integer promotion
rules convert the negative int n to a very large positive size_t value
when evaluating (n) >= sizeof(*(v)). This incorrectly bypasses the initial
bounds check.

The condition then proceeds to evaluate (v)->length >= sizeof(*(v)),
dereferencing v. Since v points to start which is beyond end, this results
in an out-of-bounds memory read (1 or 2 bytes), which could cause a kernel
panic due to a page fault on unmapped memory or trigger kasan.

This regression is exposed here in sclp_eval_selfdeftextmsg(), but also
directly in sclp_tty_receiver() when parsing untrusted sclp event buffers
from the hardware or hypervisor, where an evbuf->length smaller than
sizeof(*evbuf) causes start > end:

drivers/s390/char/sclp_tty.c:sclp_tty_receiver() {
    ...
    v = sclp_find_gds_vector(evbuf + 1, (void *) evbuf + evbuf->length,
                             GDS_ID_MDSMU);
    ...
}

>  		if (sv->key == 0x30)
>  			sclp_get_input(sv);
> +	}
>  }

[ ... ]

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

Re: [PATCH v5 1/4] s390/sclp: Introduce macro sclp_gds_for_each()

[Alexander Egorenkov]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] sclp: out-of-bounds read in sclp_gds_for_each() due to integer promotion
> --
>
> commit f292c3c2b28abf9088d99e5a8af81f738a00c9ee
> Author: Alexander Egorenkov <egorenar@linux.ibm.com>
>
> s390/sclp: Introduce macro sclp_gds_for_each()
>
> This commit introduces a macro to iterate safely over GDS {sub}vectors. It
> adds bounds checks to avoid infinite loops and out-of-bounds memory accesses
> when parsing malformed event buffers.
>
>> diff --git a/drivers/s390/char/sclp.h b/drivers/s390/char/sclp.h
>> index b31a680e08714..d22003b769f14 100644
>> --- a/drivers/s390/char/sclp.h
>> +++ b/drivers/s390/char/sclp.h
>> @@ -360,25 +360,33 @@ sclp_ascebc_str(char *str, int nr)
>>  	(machine_is_vm()) ? ASCEBC(str, nr) : ASCEBC_500(str, nr);
>>  }
>>  
>> -static inline struct gds_vector *
>> -sclp_find_gds_vector(void *start, void *end, u16 id)
>> +/* Loop over all GDS {sub}vectors in a safe manner. */
>> +#define sclp_gds_for_each(v, n, start, end)	\
>> +	for ((n) = (end) - (start), (v) = (start);	\
>> +	     (n) >= sizeof(*(v)) && (v)->length >= sizeof(*(v)) && (v)->length <= (n);	\
>
> [Severity: High]
> Does this code introduce an out-of-bounds read due to integer promotion?
>
> If a malformed SCLP event buffer is provided by the hardware with an
> evbuf->length smaller than the header size, start will be greater than end.
> This happens in callers like sclp_ocf_handler() and sclp_tty_receiver():
>
> drivers/s390/char/sclp_ocf.c:sclp_ocf_handler() {
>     ...
>     v = sclp_find_gds_vector(evbuf + 1, (void *) evbuf + evbuf->length,
>                              0x9f00);
>     ...
> }
>

This is true but the length of an SCLP event buffer should be larger than its
header.

To properly fix it i will make the parsing of event buffers in
sclp_dispatch_evbufs() more robust.

Re: [PATCH v5 1/4] s390/sclp: Introduce macro sclp_gds_for_each()

[Peter Oberparleiter]

On 17.09.2026 08:58, Alexander Egorenkov wrote:
> sclp_find_gds_{sub}vector() does not deal well with malformed event buffers
> consisting of GDS {sub}vectors. This can result in an infinite loop or
> an out-of-bounds memory read. Therefore, abort with NULL if
> * the next GDS header would exceed the given end boundary
> * the length in a GDS header contains an invalid value.
>   A valid length value in a GDS header should be at least as large
>   as the size of the corresponding GDS header (2 or 4 bytes)
>   but also not lead to exceeding the given end boundary.
> 
> Use the new macro in sclp_find_gds_{sub}vector() to iterates over entries

"iterates" => "iterate"

[...]

> --- a/drivers/s390/char/sclp.h
> +++ b/drivers/s390/char/sclp.h
> @@ -360,25 +360,33 @@ sclp_ascebc_str(char *str, int nr)
>  	(machine_is_vm()) ? ASCEBC(str, nr) : ASCEBC_500(str, nr);
>  }
>  
> -static inline struct gds_vector *
> -sclp_find_gds_vector(void *start, void *end, u16 id)
> +/* Loop over all GDS {sub}vectors in a safe manner. */
> +#define sclp_gds_for_each(v, n, start, end)	\
> +	for ((n) = (end) - (start), (v) = (start);	\
> +	     (n) >= sizeof(*(v)) && (v)->length >= sizeof(*(v)) && (v)->length <= (n);	\
> +	     (n) -= (v)->length, (v) = (void*)(v) + (v)->length)

Since "n" is likely never used by caller-provided loop-body code, would
it be acceptable to define it as a loop-scoped variable, e.g

  for (int __n = (end) - (start); ...)

This would remove the need for the caller to provide the variable.

Also an explicit cast of "end" and "start" would remove the need for the
caller to ensure that both parameters are of type void *, i.e.:

  for ((n) = (void*)(end) - (void*)(start), (v) = (void*)(start);

This would remove the need to add local void* variables as is done in
patch 2, and reduce the chance for callers using a non-void* type
parameter that would lead to incorrect calculations of "n".


-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D

Re: [PATCH v5 3/4] s390/sclp_ocf: Fix computation of length of GDS values

[Peter Oberparleiter]

On 17.09.2026 08:58, Alexander Egorenkov wrote:
> 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.

Completely subjective comment that you can safely ignore, but this could
be shortened to something like:

Fix length calculation for HMC-network and CPC names in OCF event data
by excluding the GDS subvector header size. This prevents a potential
invalid read access beyond the available GDS name data.

> Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>

Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>

-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D

Re: [PATCH v5 1/4] s390/sclp: Introduce macro sclp_gds_for_each()

[Alexander Egorenkov]

Peter Oberparleiter <oberpar@linux.ibm.com> writes:


>> -static inline struct gds_vector *
>> -sclp_find_gds_vector(void *start, void *end, u16 id)
>> +/* Loop over all GDS {sub}vectors in a safe manner. */
>> +#define sclp_gds_for_each(v, n, start, end)	\
>> +	for ((n) = (end) - (start), (v) = (start);	\
>> +	     (n) >= sizeof(*(v)) && (v)->length >= sizeof(*(v)) && (v)->length <= (n);	\
>> +	     (n) -= (v)->length, (v) = (void*)(v) + (v)->length)
>
> Since "n" is likely never used by caller-provided loop-body code, would
> it be acceptable to define it as a loop-scoped variable, e.g
>
>   for (int __n = (end) - (start); ...)
>
> This would remove the need for the caller to provide the variable.

Unfortunately, this makes all loop-scoped variables int.
The v variable would become an int. All variables in the for-loop
initialization statement must be of the same type.

An anonymous struct could be used here but i was not sure this would be acceptable:

for ( struct { int n; typeof(v) v;} __c = { (end) - (start), (start) }; ....)

What is your opinion on that option ?

>
> Also an explicit cast of "end" and "start" would remove the need for the
> caller to ensure that both parameters are of type void *, i.e.:
>
>   for ((n) = (void*)(end) - (void*)(start), (v) = (void*)(start);
>
> This would remove the need to add local void* variables as is done in
> patch 2, and reduce the chance for callers using a non-void* type
> parameter that would lead to incorrect calculations of "n".

This is a good point. Thanks.