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

[sashiko-bot@kernel.org]

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