Re: [PATCH] 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:

New issues:
- [Critical] The patch introduces an unconditional NULL pointer dereference when processing a MONWRITE_STOP_INTERVAL record.

Pre-existing issues:
- [High] MONWRITE_GEN_EVENT buffers are not properly accounted for in the global `mon_buf_count`, allowing an attacker to underflow the counter and bypass memory limits.
- [High] The global variable `mon_buf_count` is modified concurrently without global synchronization, risking corrupted counts.
--

commit d2476036fa25e1db9629a0b3a14cc2e8ed5a7d79
Author: Gerald Schaefer <gerald.schaefer@linux.ibm.com>

s390/monwriter: Reject buffer reuse with different data length

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.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/char/monwriter.c b/drivers/s390/char/monwriter.c
> index eaeb4a6384d1f..84e6deed49130 100644
> --- a/drivers/s390/char/monwriter.c
> +++ b/drivers/s390/char/monwriter.c
> @@ -123,6 +123,9 @@ static int monwrite_new_hdr(struct mon_private *monpriv)
>  			kfree(monbuf);
>  			monbuf = NULL;
>  		}
> +		/* Data with buffer reuse must not change its length */
> +		if (monbuf->hdr.datalen != monhdr->datalen)

[Severity: Critical]
Does this code cause a NULL pointer dereference?

If a user writes a valid MONWRITE_STOP_INTERVAL record, monbuf is freed and
set to NULL immediately prior to this check. Evaluating monbuf->hdr.datalen
here will unconditionally panic the kernel.

> +			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 does this code allow an underflow of
mon_buf_count?

In monwrite_new_hdr(), if a user writes a MONWRITE_GEN_EVENT header:

monwrite_new_hdr()
    ...
    if (monhdr->mon_function != MONWRITE_GEN_EVENT)
        mon_buf_count++;

the buffer is added to monpriv->list but mon_buf_count is not incremented.
If the user then skips writing the associated data and closes the file,
monwrite_close() will unconditionally decrement mon_buf_count for that
dangling buffer, underflowing the global limit counter to a negative
value and allowing a bypass of the mon_max_bufs limit.

[Severity: High]
This is also a pre-existing issue, but is the global mon_buf_count variable
safe from concurrent modifications?

It appears monwrite_close() decrements mon_buf_count while iterating over
monpriv->list without holding any global synchronization locks:

monwrite_close()
    ...
    list_for_each_entry_safe(entry, next, &monpriv->list, list) {
        ...
        mon_buf_count--;
    }

Similarly, monwrite_new_hdr() modifies it while holding only a per-file
instance thread_mutex. Can concurrent open, write, or close operations from
multiple processes corrupt the global count?

>  			return -ENOSPC;

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