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

[Mike Rapoport <rppt@kernel.org>]

On Mon, Sep 07, 2026 at 10:37:18AM +0000, sashiko-bot@kernel.org wrote:
> 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?

No, kmalloc guarantees PAGE_SIZE alignment for PAGE_SIZE allocations.

I really need Chris to merge this :)
https://github.com/masoncl/review-prompts/pull/97
 
> 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

-- 
Sincerely yours,
Mike.