Re: [PATCH v6 14/18] s390/dasd: Stamp a format label into newly formatted volumes

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

New issues:
- [Medium] Unsynchronized modification of the ese_format_quick state flag.

Pre-existing issues:
- [High] Heap buffer overflow in dasd_eckd_build_format when handling specialized format intensities.
--

--- Patch [14]: [PATCH v6 14/18] s390/dasd: Stamp a format label into newly formatted volumes ---
commit 1a0c863785cfe5455ad8a4386ef60684b5d42b93
Author: Stefan Haberland <[email protected]>

s390/dasd: Stamp a format label into newly formatted volumes

When a CDL volume is formatted, write a small on-disk label so the format
can later be recognised by the kernel. The label records a magic, a version,
whether the volume is ESE, and whether it was formatted quick (space released,
thin) or full.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 67c301e182c0f..ebffb3364c33c 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -2816,6 +2849,10 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	/* room for the label data that R4 carries in addition to its count */
> +	if (write_label)
> +		datasize += fdata->blksize;
> +

[Severity: High]
This is a pre-existing issue, but can this code cause a heap buffer overflow?

When handling specialized format intensities (e.g., 0x01 or 0x09), the size
calculation in dasd_eckd_build_format() appears to under-calculate the
required allocation for multi-track requests by missing per-track R0
allocations:

dasd_eckd_build_format() {
    ...
	case 0x01:	/* Write record zero and format track. */
	case 0x09:	/* Write record zero and format track, use cdl. */
		cplength = 2 + rpt * nr_tracks;
		if (use_prefix)
			datasize = sizeof(struct PFX_eckd_data) +
				sizeof(struct LO_eckd_data) +
				sizeof(struct eckd_count) +
				rpt * nr_tracks * sizeof(struct eckd_count);
    ...
}

However, the subsequent loop unconditionally writes nr_tracks sets of CCWs and
eckd_count structures:

dasd_eckd_build_format() {
    ...
	for (j = 0; j < nr_tracks; j++) {
        ...
		if (intensity & 0x01) {	/* write record zero */
			ect = (struct eckd_count *) data;
			data += sizeof(struct eckd_count);
        ...
			ccw++;
    ...
}

Could this write past the allocated boundaries if a user with CAP_SYS_ADMIN
invokes the BIODASDFMT ioctl with a track range spanning multiple tracks?

>  	fcp = dasd_fmalloc_request(DASD_ECKD_MAGIC, cplength, datasize, startdev);
>  	if (IS_ERR(fcp))
>  		return fcp;

[ ... ]

> @@ -3177,8 +3231,18 @@ static int dasd_eckd_format_device(struct dasd_device *base,
>  	if (fdata->start_unit == 0)
>  		dasd_ft_bias_apply(base);
>  
> -	return dasd_eckd_format_process_data(base, fdata, enable_pav, 0, NULL,
> -					     0, NULL);
> +	rc = dasd_eckd_format_process_data(base, fdata, enable_pav, 0, NULL,
> +					   0, NULL);
> +
> +	/*
> +	 * The quick-format indicator was consumed by the label stamped into
> +	 * track 0; clear it so a later format that is not preceded by a full
> +	 * space release is recorded as a full format.
> +	 */
> +	if (fdata->start_unit == 0)
> +		private->ese_format_quick = 0;

[Severity: Medium]
Is it possible for this flag modification to race with concurrent updates?

If a concurrent BIODASDRAS ioctl is executing, dasd_eckd_release_space_full()
might update this state flag without locks:

dasd_eckd_release_space_full() {
    ...
		private = device->private;
		private->ese_format_quick = 1;
    ...
}

Because dasd_eckd_format_device() clears the flag unconditionally after an
unprotected blocking operation, could it overwrite the concurrent update?

This might cause the next disk format label to be incorrectly stamped as a
'full' format rather than a 'quick' format.

> +
> +	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14