Re: [PATCH v2 2/3] s390/diag204: Replace diag204 users with new implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] s390/diag204: unlocked cache access and unlock imbalance in diag204_store()
- [Medium] s390/diag204: memory leak in diag204_probe() on simple format fallback
--

commit 5ab79951b16e6e4f69656f47d09396782867bf4f
Author: Mete Durlu <meted@linux.ibm.com>

s390/diag204: Replace diag204 users with new implementation

This commit converts the existing kernelspace diag204 users in hypfs and
sthyi to use the new diag204 interface and enables building
arch/s390/kernel/diag/diag204.c.

> diff --git a/arch/s390/hypfs/hypfs_diag.c b/arch/s390/hypfs/hypfs_diag.c
> index 08777f57bd244..c3eebe4667faf 100644
> --- a/arch/s390/hypfs/hypfs_diag.c
> +++ b/arch/s390/hypfs/hypfs_diag.c
> @@ -22,139 +22,24 @@

[ ... ]

> -static int diag204_probe(void)
> -{
> -	void *buf;
> -	int pages, rc;
> -
> -	buf = diag204_get_buffer(DIAG204_INFO_EXT, &pages);
> -	if (!IS_ERR(buf)) {
> -		if (diag204((unsigned long)DIAG204_SUBC_STIB7 |
> -			    (unsigned long)DIAG204_INFO_EXT, pages, buf) >= 0) {
> -			diag204_store_sc = DIAG204_SUBC_STIB7;
> -			diag204_set_info_type(DIAG204_INFO_EXT);
> -			goto out;
> -		}
> -		if (diag204((unsigned long)DIAG204_SUBC_STIB6 |
> -			    (unsigned long)DIAG204_INFO_EXT, pages, buf) >= 0) {
> -			diag204_store_sc = DIAG204_SUBC_STIB6;
> -			diag204_set_info_type(DIAG204_INFO_EXT);
> -			goto out;
> -		}
> -		diag204_free_buffer();
> -	}

[Severity: Medium]
Does the replacement diag204_probe() in arch/s390/kernel/diag/diag204.c (now
built via arch/s390/kernel/diag/Makefile) leak the extended format vmalloc
buffer buf when falling back to DIAG204_INFO_SIMPLE?

In the removed hypfs implementation above, diag204_free_buffer() freed the
DIAG204_INFO_EXT buffer if both DIAG204_SUBC_STIB7 and DIAG204_SUBC_STIB6
failed.

In arch/s390/kernel/diag/diag204.c:diag204_probe(), if DIAG204_SUBC_RSI
returns pages > 0 and __diag204_allocate_buffer(pages) succeeds, but both
DIAG204_SUBC_STIB7 and DIAG204_SUBC_STIB6 return a negative error code,
execution falls through to the simple format fallback and overwrites buf
without freeing the first allocation:

arch/s390/kernel/diag/diag204.c:diag204_probe() {
	if (pages > 0) {
		buf = __diag204_allocate_buffer(pages);
		if (!buf) {
			rc = -ENOMEM;
			goto fail_alloc;
		}
		type = DIAG204_INFO_EXT;
		if (diag204(DIAG204_SUBC_STIB7 | type, pages, buf) >= 0) {
			subcode = DIAG204_SUBC_STIB7;
			goto out;
		}
		if (diag204(DIAG204_SUBC_STIB6 | type, pages, buf) >= 0) {
			subcode = DIAG204_SUBC_STIB6;
			goto out;
		}
	}
	/* extended data format is not available, fallback to simple */
	pages = 1;
	buf = __diag204_allocate_buffer(pages);
}

Should vfree(buf) (or diag204_free_buffer(buf)) be called before overwriting
buf in the fallback path?

[ ... ]

> diff --git a/arch/s390/hypfs/hypfs_diag_fs.c b/arch/s390/hypfs/hypfs_diag_fs.c
> index 39621c3260103..13b5a2a4729d1 100644
> --- a/arch/s390/hypfs/hypfs_diag_fs.c
> +++ b/arch/s390/hypfs/hypfs_diag_fs.c
> @@ -303,15 +303,14 @@ int hypfs_diag_create_files(struct dentry *root)
>  	struct dentry *systems_dir, *hyp_dir;
>  	void *time_hdr, *part_hdr;
>  	void *buffer, *ptr;
> -	int i, rc, pages;
> +	int i, rc;
>  
> -	buffer = diag204_get_buffer(diag204_get_info_type(), &pages);
> +	buffer = hypfs_diag204_get_buffer();
>  	if (IS_ERR(buffer))
>  		return PTR_ERR(buffer);
> -	rc = diag204_store(buffer, pages);
> -	if (rc)
> +	rc = diag204_store(buffer, 0, 0);
> +	if (rc < 0)
>  		return rc;
> -
>  	systems_dir = hypfs_mkdir(root, "systems");
>  	if (IS_ERR(systems_dir))
>  		return PTR_ERR(systems_dir);

[ ... ]

> diff --git a/arch/s390/kernel/sthyi.c b/arch/s390/kernel/sthyi.c
> index 5eae2e25997ae..9adb26588d5b5 100644
> --- a/arch/s390/kernel/sthyi.c
> +++ b/arch/s390/kernel/sthyi.c
> @@ -305,29 +305,19 @@ static struct diag204_x_part_block *lpar_cpu_inf(struct lpar_cpu_inf *part_inf,
>  
>  static void *diag204_get_data(bool diag204_allow_busy)
>  {
> -	unsigned long subcode;
>  	void *diag204_buf;
> -	int pages, rc;
> -
> -	subcode = DIAG204_SUBC_RSI;
> -	subcode |= DIAG204_INFO_EXT;
> -	pages = diag204(subcode, 0, NULL);
> -	if (pages < 0)
> -		return ERR_PTR(pages);
> -	if (pages == 0)
> -		return ERR_PTR(-ENODATA);
> -	diag204_buf = __vmalloc_node(array_size(pages, PAGE_SIZE),
> -				     PAGE_SIZE, GFP_KERNEL, NUMA_NO_NODE,
> -				     __builtin_return_address(0));
> +	int rc;
> +
> +	if (!diag204_is_available())
> +		return ERR_PTR(-EOPNOTSUPP);
> +	if (diag204_get_subcode() != DIAG204_SUBC_STIB7)
> +		return ERR_PTR(-EOPNOTSUPP);
> +	diag204_buf = diag204_allocate_buffer();
>  	if (!diag204_buf)
>  		return ERR_PTR(-ENOMEM);
> -	subcode = DIAG204_SUBC_STIB7;
> -	subcode |= DIAG204_INFO_EXT;
> -	if (diag204_has_bif() && diag204_allow_busy)
> -		subcode |= DIAG204_BIF_BIT;
> -	rc = diag204(subcode, pages, diag204_buf);
> +	rc = diag204_store(diag204_buf, diag204_allow_busy, 0);
>  	if (rc < 0) {
> -		vfree(diag204_buf);
> +		diag204_free_buffer(diag204_buf);
>  		return ERR_PTR(rc);
>  	}
>  	return diag204_buf;

[Severity: Critical]
Can calling diag204_store() with allow_busy=0 and allow_busy_wait=0 from
hypfs_diag_create_files() or fill_dst()->diag204_get_data() (when
is_diag204_cached(sctns) is false) cause unlocked cache access and an
unbalanced mutex_unlock(&diag204_mutex)?

In arch/s390/kernel/diag/diag204.c:diag204_update_cache(), the thread
updating the cache sets g_cache.updating = true and unlocks diag204_mutex
while invoking diag204().

If a second thread concurrently calls diag204_store(buf, 0, 0) while
g_cache.updating is true:

arch/s390/kernel/diag/diag204.c:diag204_store() {
	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);
}

Because allow_busy is 0 and allow_busy_wait is 0, the loop unlocks
diag204_mutex, does not take the -EBUSY return, sleeps once (or returns
-ERESTARTSYS on a pending signal), and then exits at while (allow_busy_wait)
without holding diag204_mutex.

It then calls diag204_is_cache_valid(), diag204_update_cache(), and
memcpy(buf, READ_ONCE(g_cache.buf), ...) without holding diag204_mutex
(risking a NULL dereference or use-after-free on g_cache.buf against
diag204_set_cache()), followed by an unbalanced mutex_unlock(&diag204_mutex).

Also, in hypfs_diag_create_files(), the previous diag204_store() set
DIAG204_BIF_BIT when supported and waited interruptibly on -EBUSY. Should
hypfs_diag_create_files() pass allow_busy=1, allow_busy_wait=1 (matching
dbfs_d204_create()), and how should diag204_store() handle concurrent
updates when allow_busy=0 and allow_busy_wait=0?

[ ... ]

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