Re: [PATCH 15/19] 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] Data race on `ese_format_quick` allows concurrent space release to incorrectly taint the format label of a volume.

Pre-existing issues:
- [High] Heap buffer overflow in `dasd_eckd_build_format` when formatting multiple tracks with intensity 0x01, 0x04, 0x09, or 0x0c.
--

--- Patch [15]: [PATCH 15/19] s390/dasd: Stamp a format label into newly formatted volumes ---
commit 6ce217cc3945c138bfb4345ea55cd9cbaf16ceb9
Author: Stefan Haberland <[email protected]>

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

This commit writes a 512-byte on-disk label during CDL volume formatting to
record whether the volume is ESE and if it was a quick or full format.
The quick format state is derived from a preceding full space release.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 62c03c4787c83..25a9055b97a2c 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -2723,6 +2724,28 @@ dasd_eckd_build_check(struct dasd_device *base, struct format_data_t *fdata,
>  	return cqr;
>  }
>  
> +/* Fill the format label into a R4 record buffer, zero-padded to blksize. */
> +static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
> +					unsigned int blksize)
> +{
> +	struct dasd_eckd_private *private = device->private;
> +	struct dasd_format_label *label = data;
> +
> +	memset(label, 0, blksize);
> +	label->magic = DASD_ESE_LABEL_MAGIC;
> +	label->version = DASD_ESE_LABEL_VERSION;
> +	if (dasd_eckd_is_ese(device))
> +		label->flags |= DASD_ESE_LABEL_F_ESE;
> +	if (private->ese_format_quick)

[Severity: Medium]
Is the ese_format_quick flag safe from concurrent updates?

In dasd_eckd_fill_format_label(), private->ese_format_quick is read locklessly
while generating the CCW requests in userspace context.

If a concurrent BIODASDRAS (space release) ioctl completes and sets this flag 
to 1 via dasd_eckd_release_space_full():

dasd_eckd_release_space_full()
    private->ese_format_quick = 1;

Could the concurrent format process fetch a stale or intermediate value and
incorrectly encode DASD_ESE_LABEL_F_QUICK into the volume's on-disk label?

> +		label->flags |= DASD_ESE_LABEL_F_QUICK;
> +	else
> +		label->flags |= DASD_ESE_LABEL_F_FULL;
> +	label->blksize = blksize;
> +	label->format_tod = get_tod_clock();
> +	strscpy(label->kernel_version, init_utsname()->release,
> +		sizeof(label->kernel_version));
> +}

[ ... ]

> @@ -2774,6 +2798,15 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
>  
>  	use_prefix = base_priv->features.feature[8] & 0x01;
>  
> +	/*
> +	 * Stamp the format label into R4 of the very first track. Only for CDL
> +	 * (R4 is the first non-special record there), only when this request
> +	 * covers track 0, only for the record-writing format intensities (not
> +	 * track invalidation), and only if the track actually has an R4.
> +	 */
> +	write_label = (intensity & 0x08) && !((intensity & ~0x08) & 0x04) &&
> +		      fdata->start_unit == 0 && rpt > 3;
> +
>  	switch (intensity) {
>  	case 0x00:	/* Normal format */
>  	case 0x08:	/* Normal format, use cdl. */
> @@ -2820,6 +2853,10 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
>  		return ERR_PTR(-EINVAL);
>  	}

[Severity: High]
This is a pre-existing issue, but does this allocation lack a multiplication 
factor for nr_tracks?

Looking at dasd_eckd_build_format(), for track invalidation 
(intensity 0x04 or 0x0c), the memory calculation sets cplength statically
to 3 and datasize only provisions for a single eckd_count payload.

Later in the function, the code unconditionally iterates over nr_tracks:

for (j = 0; j < nr_tracks; j++) {
    ...
    if ((intensity & ~0x08) & 0x04) {  /* erase track */
        ect = (struct eckd_count *) data;
        data += sizeof(struct eckd_count);
        ...
        ccw++;
    }
}

If nr_tracks is greater than 1, it appears this loop will write past the 
allocated bounds of the memory chunk since the allocation size was not scaled
by nr_tracks.

Can this cause a heap buffer overflow during a privileged BIODASDFMT ioctl?

>  
> +	/* room for the label data that R4 carries in addition to its count */
> +	if (write_label)
> +		datasize += fdata->blksize;
> +
>  	fcp = dasd_fmalloc_request(DASD_ECKD_MAGIC, cplength, datasize, startdev);
>  	if (IS_ERR(fcp))
>  		return fcp;

[ ... ]

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