[PATCH 0/3] s390/dasd: fixes

[PATCH 0/3] s390/dasd: fixes

[Stefan Haberland]

Hi Jens,

please apply the following 3 patches that partially address rather
important issues in the DASD driver.
Thanks.

Jan Höppner (1):
  s390/dasd: Fix potential NULL pointer dereference

Stefan Haberland (2):
  s390/dasd: Fix path verification interrupted by concurrent
    dasd_sleep_on_immediatly
  s390/dasd: Fix undersized format-check buffer

 drivers/s390/block/dasd.c       | 14 +++++++++++++-
 drivers/s390/block/dasd_eckd.c  | 11 +++++++----
 drivers/s390/block/dasd_ioctl.c |  2 +-
 3 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.53.0

[PATCH 1/3] s390/dasd: Fix path verification interrupted by concurrent dasd_sleep_on_immediatly

[Stefan Haberland]

When all channel paths to a DASD device are lost and subsequently
recovered, the path event handler starts one IO per path via
dasd_sleep_on_immediatly() to execute read configuration data (RCD) with
high priority.

dasd_sleep_on_immediatly() works by terminating the currently running
request before inserting the new request.
If a concurrent caller, such as the attention handler
dasd_eckd_check_attention_work() or the summary unit
check handler summary_unit_check_handling_work(), also calls
dasd_sleep_on_immediatly() while a path verification RCD is in progress,
the RCD gets terminated.

The problem is that a terminated request transitions from CLEARED to
TERMINATED without going through the normal retry path in
__dasd_device_process_ccw_queue.

The RCD therefore returns -EIO, and the affected paths remain
non-operational after recovery. RCD CQRs used for path verification
already carry the DASD_CQR_VERIFY_PATH flag.

Extend _dasd_term_running_cqr() to check this flag: instead of terminating
such a request, return -EAGAIN. In dasd_sleep_on_immediatly(), loop on
-EAGAIN with a short sleep, waiting for the path verification request to
complete before inserting the new request.

This is consistent with the already indefinite wait_event() that
dasd_sleep_on_immediatly() uses for its own request, and all other callers
(attention handler, summary unit check handler, reserve/release/steal-lock)
benefit automatically without requiring changes.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 3181c06d91ce..d8d912a3b3fe 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -21,6 +21,7 @@
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
 #include <linux/vmalloc.h>
+#include <linux/delay.h>
 
 #include <asm/machine.h>
 #include <asm/ccwdev.h>
@@ -2511,6 +2512,13 @@ static inline int _dasd_term_running_cqr(struct dasd_device *device)
 	if (list_empty(&device->ccw_queue))
 		return 0;
 	cqr = list_entry(device->ccw_queue.next, struct dasd_ccw_req, devlist);
+	/*
+	 * Path verification requests must not be terminated. They are critical
+	 * for bringing paths back online. Terminating them would cause rc=-EIO
+	 * because CLEARED requests skip the retry path.
+	 */
+	if (test_bit(DASD_CQR_VERIFY_PATH, &cqr->flags))
+		return -EAGAIN;
 	rc = device->discipline->term_IO(cqr);
 	if (!rc)
 		/*
@@ -2535,7 +2543,11 @@ int dasd_sleep_on_immediatly(struct dasd_ccw_req *cqr)
 		return -EIO;
 	}
 	spin_lock_irq(get_ccwdev_lock(device->cdev));
-	rc = _dasd_term_running_cqr(device);
+	while ((rc = _dasd_term_running_cqr(device)) == -EAGAIN) {
+		spin_unlock_irq(get_ccwdev_lock(device->cdev));
+		msleep(1);
+		spin_lock_irq(get_ccwdev_lock(device->cdev));
+	}
 	if (rc) {
 		spin_unlock_irq(get_ccwdev_lock(device->cdev));
 		return rc;
-- 
2.53.0

Re: [PATCH 1/3] s390/dasd: Fix path verification interrupted by concurrent dasd_sleep_on_immediatly

[sashiko-bot]

> When all channel paths to a DASD device are lost and subsequently
> recovered, the path event handler starts one IO per path via
> dasd_sleep_on_immediatly() to execute read configuration data (RCD) with
> high priority.
> 
> dasd_sleep_on_immediatly() works by terminating the currently running
> request before inserting the new request.
> If a concurrent caller, such as the attention handler
> dasd_eckd_check_attention_work() or the summary unit
> check handler summary_unit_check_handling_work(), also calls
> dasd_sleep_on_immediatly() while a path verification RCD is in progress,
> the RCD gets terminated.
> 
> The problem is that a terminated request transitions from CLEARED to
> TERMINATED without going through the normal retry path in
> [ ... ]
> (attention handler, summary unit check handler, reserve/release/steal-lock)
> benefit automatically without requiring changes.
> 
> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH 2/3] s390/dasd: Fix potential NULL pointer dereference

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

dasd_release_space() checks the implementation of the is_ese()
discipline function before calling it to determine if a given device is
an ESE DASD.

The current usage of the logical AND operator will lead to a NULL
pointer dereference as the function is called even if the function
pointer is NULL.

Fix this by using the logical OR operator.

Fixes: 91dc4a197569 ("s390/dasd: Add new ioctl to release space")
Cc: stable@vger.kernel.org # v5.3+
Reported-by: Vasily Gorbik <gor@linux.ibm.com>
Acked-by: Eduard Shishkin <edward6@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index c85ee42732a3..e5b8b413f5ab 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -324,7 +324,7 @@ static int dasd_ioctl_check_format(struct block_device *bdev, void __user *argp)
 static int dasd_release_space(struct dasd_device *device,
 			      struct format_data_t *rdata)
 {
-	if (!device->discipline->is_ese && !device->discipline->is_ese(device))
+	if (!device->discipline->is_ese || !device->discipline->is_ese(device))
 		return -ENOTSUPP;
 	if (!device->discipline->release_space)
 		return -ENOTSUPP;
-- 
2.53.0

Re: [PATCH 2/3] s390/dasd: Fix potential NULL pointer dereference

[sashiko-bot]

> From: Jan Höppner <hoeppner@linux.ibm.com>
> 
> dasd_release_space() checks the implementation of the is_ese()
> discipline function before calling it to determine if a given device is
> an ESE DASD.
> 
> The current usage of the logical AND operator will lead to a NULL
> pointer dereference as the function is called even if the function
> pointer is NULL.
> 
> Fix this by using the logical OR operator.
> 
> Fixes: 91dc4a197569 ("s390/dasd: Add new ioctl to release space")
> Cc: stable@vger.kernel.org # v5.3+
> Reported-by: Vasily Gorbik <gor@linux.ibm.com>
> Acked-by: Eduard Shishkin <edward6@linux.ibm.com>
> Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
> Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH 3/3] 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 74fe73b5738a..d356a9f8f016 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 3/3] s390/dasd: Fix undersized format-check buffer

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but missing input validation allows an unprivileged user to trigger a massive memory allocation, leading to a stack trace or kernel panic (if panic_on_warn is enabled).
- [High] This problem wasn't introduced by this patch, but out-of-bounds array reads occur in the while loop parsing trailing zeroes in the format buffer, potentially leading to a kernel panic.
--

--- Patch [3]: [PATCH 3/3] s390/dasd: Fix undersized format-check buffer ---
commit fff320f435d672762ac313457acd2d29da83d443
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Fix undersized format-check buffer

This patch fixes an issue where a 64-bit calculation for a buffer size was
silently truncated to a 32-bit integer. By using a size_t and checking for
multiplication overflows, it prevents undersized allocations that lead to
out-of-bounds writes.

Cc: stable@vger.kernel.org #4.7

> --- 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: High]
This is a pre-existing issue, but since the track range start_unit and
stop_unit are user-controlled via the ioctl, could an unprivileged user
still trigger a massive memory allocation request here?

Without the __GFP_NOWARN flag, passing an extremely large size to kzalloc
will log an allocator warning, which can panic the kernel if panic_on_warn
is enabled.

Should we add __GFP_NOWARN to this allocation, or enforce a stricter upper
bound on trkcount?

[Severity: High]
This is also a pre-existing issue, but do we have an out-of-bounds array
read later on when evaluating the tracks from this same format buffer?

Looking at dasd_eckd_format_evaluate_tracks() called when processing
these tracks in transport mode:

drivers/s390/block/dasd_eckd.c:dasd_eckd_format_evaluate_tracks() {
    ...
            while (fmt_buffer[pos].record == 0 &&
                   fmt_buffer[pos].dl == 0) {
                if (pos++ > max_entries)
                    break;
            }
    ...
}

Since the loop condition evaluates fmt_buffer[pos] before checking if pos
exceeds max_entries, it seems we can read past the end of the allocated
fmt_buffer array.

Additionally, this out-of-bounds pos index is passed to
dasd_eckd_count_records(), resulting in another unconditional out-of-bounds
read:

drivers/s390/block/dasd_eckd.c:dasd_eckd_count_records() {
    ...
    head = fmt_buffer[start].head;
    ...
}

Could this trigger memory safety warnings or a kernel panic if the read
crosses into an unmapped page?

>  	if (!fmt_buffer)
>  		return -ENOMEM;

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