linux-kernel - Re: [PATCH 1/2] fscontext: do not consume log entries for -EMSGSIZE case

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250806055755.GF222315@ZenIV>
Date: Wed, 6 Aug 2025 06:57:55 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Aleksa Sarai <cyphar@...har.com>
Cc: Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	David Howells <dhowells@...hat.com>, Shuah Khan <shuah@...nel.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 1/2] fscontext: do not consume log entries for -EMSGSIZE
 case

On Wed, Aug 06, 2025 at 03:31:10PM +1000, Aleksa Sarai wrote:
> Userspace generally expects APIs that return EMSGSIZE to allow for them
> to adjust their buffer size and retry the operation. However, the
> fscontext log would previously clear the message even in the EMSGSIZE
> case.
> 
> Given that it is very cheap for us to check whether the buffer is too
> small before we remove the message from the ring buffer, let's just do
> that instead.
> 
> Fixes: 007ec26cdc9f ("vfs: Implement logging through fs_context")
> Cc: David Howells <dhowells@...hat.com>
> Cc: <stable@...r.kernel.org> # v5.2+
> Signed-off-by: Aleksa Sarai <cyphar@...har.com>
> ---
>  fs/fsopen.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/fsopen.c b/fs/fsopen.c
> index 1aaf4cb2afb2..f5fdaa97965b 100644
> --- a/fs/fsopen.c
> +++ b/fs/fsopen.c
> @@ -36,23 +36,25 @@ static ssize_t fscontext_read(struct file *file,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (log->head == log->tail) {
> -		mutex_unlock(&fc->uapi_mutex);
> -		return -ENODATA;
> -	}
> +	ret = -ENODATA;
> +	if (log->head == log->tail)
> +		goto err_unlock_nomsg;
>  
>  	index = log->tail & (logsize - 1);
>  	p = log->buffer[index];
> +	n = strlen(p);
> +
> +	ret = -EMSGSIZE;
> +	if (n > len)
> +		goto err_unlock_nomsg;
> +

FWIW, I would rather turn that into a helper taking log and len and
returning p or ERR_PTR(...); something like

static inline const char *fetch_message(struct fc_log *log, size_t len,
					bool *need_free)
{
	int index = log->tail & (ARRAY_SIZE(log->buffer) - 1);
	const char *p = log->buffer[index];

	if (unlikely(log->head == log->tail))
		return ERR_PTR(-ENODATA);

	n = strlen(p);
	if (unlikely(n > len))
		return ERR_PTR(-EMSGSIZE);

	log->buffer[index] = NULL;
	*need_free = log->need_free & (1 << index);
	log->need_free &= ~(1 << index);
	log->tail++;

	return p;
}

with caller being

        ret = mutex_lock_interruptible(&fc->uapi_mutex);
	if (ret < 0)
		return ret;
	p = fetch_message(log, len, &need_free);
	mutex_unlock(&fc->uapi_mutex);
	if (IS_ERR(p))
		return PTR_ERR(p);
	n = strlen(p);
	if (copy_to_user(_buf, p, n))
		n = -EFAULT;
	if (need_free)
		kfree(p);
	return n;

and that's it.