Re: [PATCH v2 3/3] s390/diag: Add diag204 IOCTL to diag misc device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] s390/diag: unlocked cache access and mutex unlock in diag204_store()
- [Medium] s390/diag: missing subcode metadata and non-UAPI types in diag204 ioctl
--

commit 8eea5b568c2725dd27a2e3675754996596fdf2e4
Author: Mete Durlu <meted@linux.ibm.com>

s390/diag: Add diag204 IOCTL to diag misc device

This commit adds ioctl handlers to the s390 diag misc device to query the
diag204 buffer length and retrieve diag204 data from userspace.

> diff --git a/arch/s390/include/uapi/asm/diag.h b/arch/s390/include/uapi/asm/diag.h
> index b7e6ccb4ff6e4..985667b2917a5 100644
> --- a/arch/s390/include/uapi/asm/diag.h
> +++ b/arch/s390/include/uapi/asm/diag.h
> @@ -22,11 +22,19 @@ struct diag310_memtop {
>  	__u64 nesting_lvl;
>  };
>  
> +struct diag204_request {
> +	__u64 address;
> +	int allow_busy;
> +	int allow_busy_wait;
> +};
> +
>  /* Diag ioctl definitions */
>  #define DIAG324_GET_PIBBUF	_IOWR(DIAG_MAGIC_STR, 0x77, struct diag324_pib)
>  #define DIAG324_GET_PIBLEN	_IOR(DIAG_MAGIC_STR, 0x78, size_t)
>  #define DIAG310_GET_STRIDE	_IOR(DIAG_MAGIC_STR, 0x79, size_t)
>  #define DIAG310_GET_MEMTOPLEN	_IOWR(DIAG_MAGIC_STR, 0x7a, size_t)
>  #define DIAG310_GET_MEMTOPBUF	_IOWR(DIAG_MAGIC_STR, 0x7b, struct diag310_memtop)
> +#define DIAG204_GET_LEN		_IOR(DIAG_MAGIC_STR, 0x7c, size_t)
> +#define DIAG204_GET_BUF		_IOWR(DIAG_MAGIC_STR, 0x7d, struct diag204_request)
>  
>  #endif /* __S390_UAPI_ASM_DIAG_H */

[Severity: Medium]
During boot, diag204_probe() in arch/s390/kernel/diag/diag204.c selects
between subcodes 4, 6, and 7 and sets g_info.subcode and g_info.type
(simple vs. extended format). When userspace calls diag_ioctl() ->
diag204_buf(), the raw hardware buffer is copied to req.address via
copy_to_user() without exposing g_info.subcode (diag204_get_subcode()) or
g_info.type (diag204_get_info_type()).

Unlike dbfs_d204_create() in arch/s390/hypfs/hypfs_diag.c, which prepends
struct dbfs_d204_hdr containing d204->hdr.sc = diag204_get_subcode(), the
raw hardware headers (struct diag204_info_blk_hdr and
struct diag204_x_info_blk_hdr) do not contain a subcode or format field.
Because simple and extended formats have different header and entry sizes
(and subcodes 6 and 7 share the extended header layout while differing in
secondary processor reporting), how can userspace determine which subcode
or format was used to populate the buffer returned by diag204_buf()?

Also, in struct diag204_request, should allow_busy and allow_busy_wait use
fixed-width types (__u32 or __u64) instead of raw int, and should the
ioctl definition use _IOW instead of _IOWR since diag204_buf() only reads
struct diag204_request?

> diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
> index 7f366ea12a673..3450a013258fd 100644
> --- a/arch/s390/kernel/diag/diag204.c
> +++ b/arch/s390/kernel/diag/diag204.c

[ ... ]

> @@ -220,6 +222,45 @@ int diag204_store(void *buf, int allow_busy, int allow_busy_wait)
>  	return rc;
>  }

[ ... ]

> +long diag204_buf(unsigned long arg)
> +{
> +	struct diag204_request __user *udata;
> +	int rc, allow_busy, allow_busy_wait;
> +	u64 address;
> +	void *buf;
> +
> +	if (!diag204_is_available())
> +		return -EOPNOTSUPP;
> +	udata = (struct diag204_request __user *)arg;
> +	if (get_user(address, &udata->address))
> +		return -EFAULT;
> +	if (get_user(allow_busy, &udata->allow_busy))
> +		return -EFAULT;
> +	if (get_user(allow_busy_wait, &udata->allow_busy_wait))
> +		return -EFAULT;
> +	buf = diag204_allocate_buffer();
> +	if (!buf)
> +		return -ENOMEM;
> +	rc = diag204_store(buf, allow_busy, allow_busy_wait);

[Severity: Critical]
In diag_ioctl() -> diag204_buf() -> diag204_store(), user-controlled
allow_busy and allow_busy_wait values are passed directly into
diag204_store(). What happens if a caller passes allow_busy == 0 and
allow_busy_wait == 0 while another thread is concurrently updating the
cache (g_cache.updating is true)?

Looking at arch/s390/kernel/diag/diag204.c:diag204_store():

	if (!allow_busy && allow_busy_wait)
		return -EOPNOTSUPP;
	if (!buf)
		return -ENOMEM;
	rc = 0;
	do {
		mutex_lock(&diag204_mutex);
		/* Exit loop with lock taken if there is no concurrent update */
		if (!READ_ONCE(g_cache.updating))
			break;
		mutex_unlock(&diag204_mutex);
		if (allow_busy && !allow_busy_wait)
			return -EBUSY;
		if (signal_pending(current))
			return -ERESTARTSYS;
		schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
	} while (allow_busy_wait);
	if (!diag204_is_cache_valid(DIAG204_CACHE_EXPIRY))
		rc = diag204_update_cache(allow_busy, allow_busy_wait);
	if (rc >= 0)
		memcpy(buf, READ_ONCE(g_cache.buf), diag204_get_buf_size());
	mutex_unlock(&diag204_mutex);

When g_cache.updating is true and both allow_busy and allow_busy_wait are
0, the loop unlocks diag204_mutex, sleeps in
schedule_timeout_interruptible(), and then terminates because
allow_busy_wait is 0.

Doesn't this leave diag204_mutex unlocked for the remainder of
diag204_store(), triggering lockdep assertions in diag204_is_cache_valid()
and diag204_update_cache(), racing with vfree() of g_cache.buf in
diag204_set_cache() and diag204_work_fn() during memcpy() (or reading a
null g_cache.buf if the first cache fill has not completed), and calling
mutex_unlock(&diag204_mutex) without holding the lock?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-diag204ioctl-v2-0-7e5a9d1cd21a@linux.ibm.com?part=3