[PATCH v2] s390/monwriter: Reject buffer reuse with different data length

[PATCH v2] s390/monwriter: Reject buffer reuse with different data length

[Gerald Schaefer]

When data buffers are reused, e.g. for interval sample records, the
first record determines the data length, and the size of the buffer for
user copy. Current monwriter code does not check if the data length was
changed for subsequent records, which also would never happen for valid
user programs.

However, a malicious user could change the data length, resulting in out
of bounds user copy to the kernel buffer, and memory corruption. By
default, the monwriter misc device is created with root-only permissions,
so practical impact is typically low.

Fix this by checking for changed data length and rejecting such records.

Cc: stable@vger.kernel.org
Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
---
 drivers/s390/char/monwriter.c | 3 +++
 1 file changed, 3 insertions(+)

---
Changes in v2:
- Move check into "else if", to avoid NULL pointer dereference after
  MONWRITE_STOP_INTERVAL.

diff --git a/drivers/s390/char/monwriter.c b/drivers/s390/char/monwriter.c
index eaeb4a6384d1..ecf121a87f88 100644
--- a/drivers/s390/char/monwriter.c
+++ b/drivers/s390/char/monwriter.c
@@ -122,6 +122,9 @@ static int monwrite_new_hdr(struct mon_private *monpriv)
 			kfree(monbuf->data);
 			kfree(monbuf);
 			monbuf = NULL;
+		} else if (monbuf->hdr.datalen != monhdr->datalen) {
+			/* Data with buffer reuse must not change its length */
+			return -EINVAL;
 		}
 	} else if (monhdr->mon_function != MONWRITE_STOP_INTERVAL) {
 		if (mon_buf_count >= mon_max_bufs)

base-commit: f31c00c377ccf07c85442712f7c940a855cb3371
-- 
2.53.0

Re: [PATCH v2] s390/monwriter: Reject buffer reuse with different data length

[sashiko-bot]

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

Pre-existing issues:
- [High] Stranded MONWRITE_GEN_EVENT buffers cause mon_buf_count to underflow on close, allowing unbounded kernel memory allocation.
- [High] The global variable mon_buf_count is updated concurrently without any synchronization, leading to a data race.
- [High] Asynchronous teardown failure in monwrite_diag leads to hypervisor Use-After-Free/Information Leak.
--

commit eaf3c589ff6055b82f62fc5d169905bbb8fd5129
Author: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
Subject: s390/monwriter: Reject buffer reuse with different data length

This commit fixes an issue where reusing data buffers with different data
lengths could lead to an out-of-bounds user copy. It introduces a check in
monwrite_new_hdr() to reject interval sample records if the new data length
does not match the original buffer length.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/char/monwriter.c b/drivers/s390/char/monwriter.c
> index eaeb4a6384d1f..ecf121a87f88f 100644
> --- a/drivers/s390/char/monwriter.c
> +++ b/drivers/s390/char/monwriter.c
> @@ -122,6 +122,9 @@ static int monwrite_new_hdr(struct mon_private *monpriv)
>  			kfree(monbuf->data);

[Severity: High]
This is a pre-existing issue, but does this code risk a hypervisor
use-after-free or information leak if monwrite_diag() fails?

If kmalloc() fails inside monwrite_diag() during the teardown phase, it
returns -ENOMEM without issuing the APPLDATA_STOP_REC instruction to the
hypervisor. Because the error is ignored here and the data buffer is freed
unconditionally, the hypervisor will continue reading the freed kernel
memory.

>  			kfree(monbuf);
>  			monbuf = NULL;
> +		} else if (monbuf->hdr.datalen != monhdr->datalen) {
> +			/* Data with buffer reuse must not change its length */
> +			return -EINVAL;
>  		}
>  	} else if (monhdr->mon_function != MONWRITE_STOP_INTERVAL) {
>  		if (mon_buf_count >= mon_max_bufs)

[Severity: High]
This is a pre-existing issue, but is there a data race on the global
mon_buf_count variable?

Multiple threads opening /dev/monwriter can concurrently execute
monwrite_write() or monwrite_close() using their own per-file-descriptor
thread_mutex. Because mon_buf_count is updated and checked without a
global lock, concurrent modifications can corrupt the counter and allow
the buffer limit to be bypassed.

[Severity: High]
This is a pre-existing issue, but can this logic lead to a mon_buf_count
underflow?

If a user writes a MONWRITE_GEN_EVENT header with a data length of 0,
monwrite_write() will skip calling monwrite_new_data(), leaving the buffer
permanently on the monpriv->list. Since mon_buf_count is not incremented
for MONWRITE_GEN_EVENT buffers:

drivers/s390/char/monwriter.c:monwrite_new_hdr() {
        ...
        if (monhdr->mon_function != MONWRITE_GEN_EVENT)
                mon_buf_count++;
        ...
}

When the file descriptor is closed, monwrite_close() iterates the list
and unconditionally decrements mon_buf_count for every entry:

drivers/s390/char/monwriter.c:monwrite_close() {
        ...
        mon_buf_count--;
        list_del(&entry->list);
        ...
}

This underflows the global counter into negative values, effectively
disabling the mon_max_bufs limit check for future writes.

>  			return -ENOSPC;
[ ... ]

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