* Re: [PATCH] s390/dasd: Fix undersized format-check buffer
2026-07-22 10:37 [PATCH] s390/dasd: Fix undersized format-check buffer Stefan Haberland
@ 2026-07-22 10:52 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-22 10:52 UTC (permalink / raw)
To: Stefan Haberland
Cc: linux-s390, Vasily Gorbik, Heiko Carstens, Alexander Gordeev
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
^ permalink raw reply [flat|nested] 2+ messages in thread