Re: [PATCH v2 07/13] s390/idals: Use kmalloc() for IDAL data buffers

[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 / I/O Error due to kmalloc

Pre-existing issues:
- [High] KASAN out-of-bounds read in idal_buffer_from_user() when count is 0
--

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

s390/idals: Use kmalloc() for IDAL data buffers

idal_buffer_alloc() allocates the data chunks of an IDAL buffer that is
used for channel I/O. These buffers can be allocated with kmalloc() as
there's nothing special about them to go directly to the page allocator.
kmalloc() provides a better API that does not require ugly casts.

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

> diff --git a/arch/s390/include/asm/idals.h b/arch/s390/include/asm/idals.h
> index 06e1ec2afd5af..213f430a8d33a 100644
> --- a/arch/s390/include/asm/idals.h
> +++ b/arch/s390/include/asm/idals.h

[ ... ]

> @@ -147,9 +148,11 @@ static inline struct idal_buffer *idal_buffer_alloc(size_t size, int page_order)
>  			ib->data[i] = dma64_add(ib->data[i - 1], IDA_BLOCK_SIZE);
>  			continue;
>  		}
> -		vaddr = (void *)__get_free_pages(GFP_KERNEL, page_order);
> +		vaddr = kmalloc(PAGE_SIZE << page_order, GFP_KERNEL);

[Severity: Critical]
Does replacing __get_free_pages() with kmalloc() break the page-alignment
guarantee required for IDAWs?

The s390 architecture mandates that while the first IDAW (Indirect Data
Address Word) can specify any byte offset, all subsequent IDAWs must be
strictly aligned to a 2K or 4K boundary. Furthermore, hardware transfers
data on the first IDAW only up to the next physical 2K/4K boundary.

When CONFIG_SLUB_DEBUG is enabled, kmalloc redzoning shifts the payload away
from a page boundary. This unaligned address is stored in ib->data[0], and
dma64_add() propagates it to subsequent IDAWs:

arch/s390/include/asm/idals.h:idal_buffer_alloc() {
    ...
		if (i & (nr_chunks - 1)) {
			ib->data[i] = dma64_add(ib->data[i - 1], IDA_BLOCK_SIZE);
			continue;
		}
    ...
}

Will this unaligned base violate the hardware alignment requirement and cause
a channel-program check, as well as silently corrupt data since hardware
splits the transfer at the physical 4K boundary?

>  		if (!vaddr)
>  			goto error;
> +		/* Only DMA addresses are retained in ib->data. */
> +		kmemleak_ignore(vaddr);
>  		ib->data[i] = virt_to_dma64(vaddr);
>  	}
>  	return ib;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible to trigger a
KASAN out-of-bounds read in idal_buffer_from_user() when count is 0?

In drivers/s390/char/fs3270.c, fs3270_write() takes a count parameter from
the user and passes it directly to the allocator without checking if it is 0:

drivers/s390/char/fs3270.c:fs3270_write() {
    ...
	ib = idal_buffer_alloc(count, 0);
	if (IS_ERR(ib))
		return -ENOMEM;
    ...
		if (idal_buffer_from_user(ib, data, count) == 0) {
    ...
}

If count is 0, idal_buffer_alloc() calculates nr_ptrs = 0 and allocates
exactly sizeof(struct idal_buffer) with 0 elements for the data flex-array.

Then idal_buffer_from_user() accesses the first element of this array
unconditionally:

arch/s390/include/asm/idals.h:idal_buffer_from_user() {
    ...
	for (i = 0; count > IDA_BLOCK_SIZE; i++) {
        ...
	}
	vaddr = dma64_to_virt(ib->data[i]);
	return copy_from_user(vaddr, from, count);
}

When count is 0, the loop is skipped and it evaluates ib->data[0]. Since the
array was allocated with 0 elements, does this read 8 bytes past the allocated
16-byte boundary of the struct?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-s390-cio-ready-v2-0-e931086fdde1@kernel.org?part=7