Re: [PATCH 01/13] s390/chsc: Use kzalloc() for CUBs

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Hardware Alignment Violation

Pre-existing issues:
- [High] DMA Use-After-Free
--

commit c0b30acb33e9033572c3388a78f4e8cd3bf3f838
Author: Mike Rapoport (Microsoft) <rppt@kernel.org>

s390/chsc: Use kzalloc() for CUBs

cub_alloc() allocates the channel measurement unit blocks and their
extended counterparts.

This memory can be allocated with kmalloc() as there's nothing special
about it to go directly to the page allocator.

Link: https://lore.kernel.org/all/635405e4-9423-4a25-a6e7-e03c8ea0bcbe@redhat.com

> diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
> index 9689f722c863c..60a4a7e6086ce 100644
> --- a/drivers/s390/cio/chsc.c
> +++ b/drivers/s390/cio/chsc.c
> @@ -931,12 +931,12 @@ static int cub_alloc(struct channel_subsystem *css)
>  	int i;
>  
>  	for (i = 0; i < CSS_NUM_CUB_PAGES; i++) {
> -		css->cub[i] = (void *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
> +		css->cub[i] = kzalloc(PAGE_SIZE, GFP_KERNEL | GFP_DMA);

[Severity: Critical]
Does replacing get_zeroed_page() with kzalloc() break hardware alignment
requirements?

The original get_zeroed_page() allocated directly from the page allocator,
which strictly guarantees 4KB page alignment. kzalloc(PAGE_SIZE) allocates
from the generic slab allocator, which does not guarantee page alignment
when slab debugging (SLUB_DEBUG) or KASAN is enabled due to redzones and
padding.

When these pointers are passed to the hardware:

drivers/s390/cio/chsc.c:__chsc_do_secm() {
    ...
    for (i = 0; i < CSS_NUM_CUB_PAGES; i++)
        secm_area->cub[i] = (__force dma64_t)virt_to_dma32(css->cub[i]);
    ...
}

The s390 Store Extended Channel Measurement (SECM) hardware expects the
measurement unit blocks to be strictly aligned to a 4KB boundary. If the
pointer from kzalloc() is not aligned, the hardware will either reject the
instruction or mask off the lower 12 bits, writing DMA data to the start
of the slab page instead of the object offset. This can silently overwrite
redzones or preceding objects in the slab.

Can we continue to use a page-aligned allocator for these DMA blocks?

>  		if (!css->cub[i])
>  			return -ENOMEM;
>  	}
[ ... ]
> @@ -949,11 +949,11 @@ static void cub_free(struct channel_subsystem *css)
>  	int i;
>  
>  	for (i = 0; i < CSS_NUM_CUB_PAGES; i++) {
> -		free_page((unsigned long)css->cub[i]);
> +		kfree(css->cub[i]);
>  		css->cub[i] = NULL;
>  	}
>  	for (i = 0; i < CSS_NUM_ECUB_PAGES; i++) {
> -		free_page((unsigned long)css->ecub[i]);
> +		kfree(css->ecub[i]);
>  		css->ecub[i] = NULL;
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can this error path lead to a
DMA use-after-free?

In the rollback path of chsc_secm() which eventually calls cub_free():

drivers/s390/cio/chsc.c:chsc_secm() {
    ...
    if (!ret) {
        css->cm_enabled = enable;
        if (css->cm_enabled) {
            ret = chsc_add_cmg_attr(css);
            if (ret) {
                __chsc_do_secm(css, 0);
                css->cm_enabled = 0;
            }
    ...
out:
    if (!css->cm_enabled)
        cub_free(css);

If __chsc_do_secm(css, 0) fails (for example, returning -EBUSY because the
channel subsystem is busy), the hardware ignores the stop command and
continues performing DMA writes to the allocated memory blocks.

However, the code ignores the return value, unconditionally clears
css->cm_enabled, and frees the DMA memory via cub_free(). The hardware will
then continue to update the measurement blocks in the freed memory.

Should this path handle the failure of __chsc_do_secm() before freeing the
memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-s390-cio-ready-v1-0-ca7f39806234@kernel.org?part=1