[PATCH] s390/dasd: Fix undersized format-check buffer

[PATCH] s390/dasd: Fix undersized format-check buffer

[Stefan Haberland]

fmt_buffer_size in dasd_eckd_check_device_format() is declared as
int, even though one of the multiplicands, sizeof(struct eckd_count),
is a size_t. The expression

    trkcount * rpt_max * sizeof(struct eckd_count)

is therefore correctly evaluated at 64-bit width, but the result is
silently truncated when it is stored back into the 32-bit
fmt_buffer_size variable. For a sufficiently large track range
(start_unit/stop_unit are caller-controlled) this truncation
yields a buffer size far smaller than the number of tracks actually
requested. kzalloc() then succeeds with an undersized allocation,
while the subsequent channel program build still operates on the
untruncated track count and writes past the end of that buffer.

Compute the buffer size with check_mul_overflow() and keep it in a
size_t, so that a value that no longer fits results in -EINVAL
instead of a silently truncated allocation size.

Fixes: 8fd575200db5 ("s390/dasd: Add new ioctl BIODASDCHECKFMT")
Cc: stable@vger.kernel.org #4.7
Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 073795ea7cd0..3d015ef48d2c 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -20,6 +20,7 @@
 #include <linux/seq_file.h>
 #include <linux/uaccess.h>
 #include <linux/io.h>
+#include <linux/overflow.h>
 
 #include <asm/css_chars.h>
 #include <asm/machine.h>
@@ -3475,11 +3476,11 @@ static int dasd_eckd_check_device_format(struct dasd_device *base,
 {
 	struct dasd_eckd_private *private = base->private;
 	struct eckd_count *fmt_buffer;
-	struct irb irb;
+	size_t fmt_buffer_size;
+	unsigned int trkcount;
 	int rpt_max, rpt_exp;
-	int fmt_buffer_size;
+	struct irb irb;
 	int trk_per_cyl;
-	int trkcount;
 	int tpm = 0;
 	int rc;
 
@@ -3490,7 +3491,9 @@ static int dasd_eckd_check_device_format(struct dasd_device *base,
 	rpt_exp = recs_per_track(&private->rdc_data, 0, cdata->expect.blksize);
 
 	trkcount = cdata->expect.stop_unit - cdata->expect.start_unit + 1;
-	fmt_buffer_size = trkcount * rpt_max * sizeof(struct eckd_count);
+	if (check_mul_overflow(trkcount, rpt_max, &fmt_buffer_size) ||
+	    check_mul_overflow(fmt_buffer_size, sizeof(struct eckd_count), &fmt_buffer_size))
+		return -EINVAL;
 
 	fmt_buffer = kzalloc(fmt_buffer_size, GFP_KERNEL | GFP_DMA);
 	if (!fmt_buffer)
-- 
2.53.0

Re: [PATCH] s390/dasd: Fix undersized format-check buffer

[sashiko-bot]

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

Pre-existing issues:
- [High] Mismatched allocation and free functions in the error path of `dasd_eckd_build_check_tcw()` cause a guaranteed NULL pointer dereference.
- [High] In FICON transport mode (`tpm`), the `while` loop skipping trailing zeros causes out-of-bounds reads on the `fmt_buffer` array.
- [High] The user-provided `result` field in `format_check_t` is uninitialized by the kernel, allowing a logic bypass that causes an out-of-bounds memory read and Information Leak.
- [Medium] Unprivileged users can trigger a kernel warning splat (DoS) via `BIODASDCHECKFMT` ioctl due to missing bounds check before a massive `kzalloc`.
- [Medium] Device reference counts are permanently leaked on the error paths of request builder functions.
--

--- Patch [1]: [PATCH] s390/dasd: Fix undersized format-check buffer ---
commit dfd4c2437bd8554cd6e6af49167ed3eb732910da
Author: Stefan Haberland <sth@linux.ibm.com>
Date:   Wed Jul 22 10:37:04 2026 +0000

    s390/dasd: Fix undersized format-check buffer
    
    fmt_buffer_size in dasd_eckd_check_device_format() is declared as
    int, even though one of the multiplicands, sizeof(struct eckd_count),
    is a size_t. Compute the buffer size with check_mul_overflow() and
    keep it in a size_t to prevent silent truncation and resulting memory
    corruption.

    Cc: stable@vger.kernel.org #4.7

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 74fe73b5738ab..d356a9f8f016f 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -3490,7 +3491,9 @@ static int dasd_eckd_check_device_format(struct dasd_device *base,
>  	rpt_exp = recs_per_track(&private->rdc_data, 0, cdata->expect.blksize);
>  
>  	trkcount = cdata->expect.stop_unit - cdata->expect.start_unit + 1;
> -	fmt_buffer_size = trkcount * rpt_max * sizeof(struct eckd_count);
> +	if (check_mul_overflow(trkcount, rpt_max, &fmt_buffer_size) ||
> +	    check_mul_overflow(fmt_buffer_size, sizeof(struct eckd_count), &fmt_buffer_size))
> +		return -EINVAL;
>  
>  	fmt_buffer = kzalloc(fmt_buffer_size, GFP_KERNEL | GFP_DMA);

[Severity: Medium]
This is a pre-existing issue, but missing a CAP_SYS_ADMIN check allows
unprivileged users to trigger a warning splat here.

Looking at drivers/s390/block/dasd_ioctl.c:dasd_ioctl_check_format(), cdata
is populated from user space without a capability check:

    if (copy_from_user(&cdata, argp, sizeof(cdata))) {
        rc = -EFAULT;
        goto out_err;
    }

This allows a user to control start_unit and stop_unit to request an
extremely large allocation size. If the requested size exceeds
KMALLOC_MAX_SIZE, this kzalloc() will fail and throw a WARN_ON_ONCE splat
because it lacks __GFP_NOWARN.

Since the sanity checks in dasd_eckd_format_sanity_checks() are performed
later, should this ioctl require CAP_SYS_ADMIN like BIODASDFMT does, or
should the size be capped earlier to avoid spamming OOM warnings?

>  	if (!fmt_buffer)
>  		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
dasd_eckd_build_check_tcw() use the wrong free function?

If dasd_eckd_build_check_tcw() fails, it branches to out_err:

drivers/s390/block/dasd_eckd.c:dasd_eckd_build_check_tcw() {
    ...
    cqr = dasd_fmalloc_request(DASD_ECKD_MAGIC, 0, itcw_size, startdev);
    ...
out_err:
    dasd_sfree_request(cqr, startdev);
    return ERR_PTR(rc);
}

Because dasd_fmalloc_request() was used, cqr->mem_chunk is NULL. However,
dasd_sfree_request() passes this to dasd_free_chunk():

drivers/s390/block/dasd_int.h:dasd_free_chunk() {
    chunk = (struct dasd_mchunk *) ((char *) mem - sizeof(struct dasd_mchunk));
    ...
}

Will this cause invalid pointer arithmetic and a subsequent NULL pointer
dereference panic?

[Severity: Medium]
This is a pre-existing issue, but does the same error path leak the device
reference count?

Earlier in dasd_eckd_build_check_tcw(), the reference count is incremented:

drivers/s390/block/dasd_eckd.c:dasd_eckd_build_check_tcw() {
    ...
    start_priv->count++;
    ...
}

If an error occurs, out_err releases the request but does not decrement
start_priv->count. Can this result in a permanent reference leak?

[Severity: High]
This isn't a bug introduced by this patch, but is there an out-of-bounds
read when skipping trailing zeros?

In drivers/s390/block/dasd_eckd.c:dasd_eckd_format_evaluate_tracks(), the
while loop checks the array content before validating the bounds:

    while (fmt_buffer[pos].record == 0 &&
           fmt_buffer[pos].dl == 0) {
        if (pos++ > max_entries)
            break;
    }

When pos reaches max_entries, it evaluates fmt_buffer[pos] before the break
condition is checked. Furthermore, the break condition executes pos++ again,
leaving pos pointing up to 2 elements out of bounds.

This out-of-bounds index is then passed to dasd_eckd_count_records() as
start:

    for (i = start; i < max; i++) {
        if (i > start) {
            if ((fmt_buffer[i].head == head && ...

Could this trigger an out-of-bounds read on kernel heap memory?

[Severity: High]
This is a pre-existing issue, but does an uninitialized user space value
cause a logic bypass and information leak?

In drivers/s390/block/dasd_ioctl.c:dasd_ioctl_check_format(), cdata is
copied from user space without kernel initialization of cdata->result:

    if (copy_from_user(&cdata, argp, sizeof(cdata))) {
        ...

Later, in drivers/s390/block/dasd_eckd.c:dasd_eckd_format_evaluate_tracks(),
the index correction relies on this result field:

    if (!cdata->result) {
        i--;
        pos--;
    }

If a user intentionally provides a non-zero cdata->result, this bypasses the
index decrement. The loop leaves pos incremented out of bounds, and the
out-of-bounds data is copied:

    cdata->rec = fmt_buffer[pos].record;
    cdata->blksize = fmt_buffer[pos].dl;
    cdata->key_length = fmt_buffer[pos].kl;

Since cdata is then copied back to user space, could this allow unprivileged
users to leak kernel heap contents?

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