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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] s390/diag: diag204_store() fails with -EINVAL when BIF is unavailable
--

commit 24db640b46c74f692d9871eb2b56c29382e3bc2f
Author: Mete Durlu <meted@linux.ibm.com>

s390/diag: Add diag204 IOCTL to diag misc device

This commit adds the DIAG204_GET_SUBCODE, DIAG204_GET_LEN, and
DIAG204_GET_BUF ioctls to the /dev/diag misc device to allow userspace to
retrieve diagnose 204 data.

> diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
> index f447bfaf756b4..237303aa44d6f 100644
> --- a/arch/s390/kernel/diag/diag204.c
> +++ b/arch/s390/kernel/diag/diag204.c
[ ... ]
> @@ -222,6 +224,54 @@ 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: Medium]
What happens on systems where diagnose 204 is available, but the busy
indication facility (g_info.has_bif) is not supported, when a caller passes a
non-zero allow_busy value?

In arch/s390/kernel/diag/diag204.c, diag204_update_cache() is written to fall
back to a regular diagnose 204 call when g_info.has_bif is false:

    subcode = g_info.type | g_info.subcode;
    if (g_info.has_bif && allow_busy)
            subcode |= DIAG204_BIF_BIT;

However, earlier in the series, diag204_store() in
arch/s390/kernel/diag/diag204.c rejects any call with allow_busy set when
g_info.has_bif is false:

    if (allow_busy && !g_info.has_bif)
            return -EINVAL;

Because arch/s390/include/uapi/asm/diag.h does not provide an ioctl for
userspace to query whether BIF is supported, a userspace caller invoking
ioctl(fd, DIAG204_GET_BUF, &req) -> diag_ioctl() -> diag204_buf() ->
diag204_store() with allow_busy set has no way to know upfront that it must
pass allow_busy = 0 on machines without BIF.

In addition, the in-kernel callers converted earlier in the series pass
allow_busy = 1 unconditionally when BIF is not supported:

- arch/s390/hypfs/hypfs_diag.c: dbfs_d204_create() calls
  diag204_store(d204->buf, 1, 1)
- arch/s390/hypfs/hypfs_diag_fs.c: hypfs_diag_create_files() calls
  diag204_store(buffer, 1, 1)
- arch/s390/kernel/sthyi.c: diag204_get_data() calls
  diag204_store(diag204_buf, diag204_allow_busy, 0) where
  diag204_allow_busy = is_diag204_cached(sctns) is true after the first cache
  fill

Should diag204_store() allow allow_busy when !g_info.has_bif and let
diag204_update_cache() ignore it (or clamp allow_busy and allow_busy_wait to 0
when !g_info.has_bif) instead of returning -EINVAL?

> +	if (rc < 0) {
> +		diag204_free_buffer(buf);
> +		return rc;
> +	}
> +	rc = copy_to_user((void __user *)address, buf,
> +			  diag204_get_buf_size());
> +	diag204_free_buffer(buf);
> +	return rc ? -EFAULT : 0;
> +}

[ ... ]

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