Re: [PATCH v7 1/5] s390/sclp: Make parsing of received event buffers more robust

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] s390/sclp: TOCTOU double fetch of sccb->length and evbuf->length
--

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

s390/sclp: Make parsing of received event buffers more robust

Validate header fields of SCCB and event buffers to protect from potential
out-of-range memory accesses which might be caused by malformed hardware
responses.

> diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c
> index 98e334724a627..5a4e4001dfa8d 100644
> --- a/drivers/s390/char/sclp.c
> +++ b/drivers/s390/char/sclp.c
> @@ -532,8 +532,7 @@ EXPORT_SYMBOL(sclp_add_request);
>  
>  /* Dispatch events found in request buffer to registered listeners. Return 0
>   * if all events were dispatched, non-zero otherwise. */
> -static int
> -sclp_dispatch_evbufs(struct sccb_header *sccb)
> +static int sclp_dispatch_evbufs(struct sccb_header *sccb)
>  {
>  	unsigned long flags;
>  	struct evbuf_header *evbuf;
> @@ -542,14 +541,25 @@ sclp_dispatch_evbufs(struct sccb_header *sccb)
>  	int offset;
>  	int rc;
>  
> +	/* Check for malformed hardware response */
> +	if (sccb->length > PAGE_SIZE)
> +		return -EINVAL;
> +

[Severity: High]
In drivers/s390/char/sclp.c:sclp_dispatch_evbufs(), is it possible for a
malicious hypervisor to mutate sccb->length after this initial validation?

Since sccb points to DMA shared memory, this validation is performed outside
the sclp_lock.

>  	spin_lock_irqsave(&sclp_lock, flags);
>  	rc = 0;
>  	for (offset = sizeof(struct sccb_header); offset < sccb->length;
>  	     offset += evbuf->length) {

[Severity: High]
Does spin_lock_irqsave() act as a full compiler barrier here, forcing
sccb->length and evbuf->length to be re-fetched from shared memory on each loop
iteration?

If a malicious hypervisor mutates the lengths (e.g., setting sccb->length >
PAGE_SIZE or evbuf->length = 0) between the initial bounds checks and these
re-fetches, could it bypass the mitigation and cause out-of-bounds memory
reads or infinite loops?

> -		evbuf = (struct evbuf_header *) ((addr_t) sccb + offset);
>  		/* Check for malformed hardware response */
> -		if (evbuf->length == 0)
> +		if (offset + sizeof(struct evbuf_header) > sccb->length) {
> +			rc = -EINVAL;
>  			break;
> +		}
> +		evbuf = (struct evbuf_header *) ((addr_t) sccb + offset);
> +		if (evbuf->length < sizeof(*evbuf) ||
> +		    offset + evbuf->length > sccb->length) {

[Severity: High]
Do subsequent re-fetches of sccb->length and evbuf->length in these bounds
checks continue to expose a double-fetch TOCTOU window?

Because memory barriers like function calls or locking/unlocking occur during
the rest of the loop, the compiler may be forced to repeatedly fetch these
values from untrusted shared memory.

> +			rc = -EINVAL;
> +			break;
> +		}

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