Received-SPF: pass (google.com: domain of linux-kernel+bounces-29255-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 17 Jan 2024 12:07:43 -0500
From: Johannes Weiner <hannes@cmpxchg.org>
To: Yi Tao <escape@linux.alibaba.com>
Cc: surenb@google.com, peterz@infradead.org, linux-kernel@vger.kernel.org,
	Miles Chen <miles.chen@mediatek.com>
Subject: Re: [PATCH] sched/psi: Fix the bug where the last character is
 overwritten
Message-ID: <20240117170743.GD939255@cmpxchg.org>
References: <a33440f33d42aab66ad4120303ecbe8dca401d5c.1705490349.git.escape@linux.alibaba.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a33440f33d42aab66ad4120303ecbe8dca401d5c.1705490349.git.escape@linux.alibaba.com>

On Wed, Jan 17, 2024 at 07:26:01PM +0800, Yi Tao wrote:
> The buffer buf in psi_write has only 32 bytes, and to ensure the correct
> parsing of the string, it needs to be terminated with '\0', which means
> users can input no more than 31 characters. When the user inputs fewer
> than 31 characters, buf_size equals nbytes, which causes the last
> character entered by the user to be overwritten by '\0', affecting the
> parsing results.
> 
> Here is a specific example.
> 
> $echo -n "some 500000 1000000" > /proc/pressure/cpu
> $bash: echo: write error: Invalid argument
> 
> Because the last character is overwritten, the value obtained by sscanf
> parsing is 500000 and 100000; window_us is missing a zero, hence the
> return of -EINVAL.
> 
> The reason 'echo' without the '-n' flag can be parsed correctly is
> because the last character that gets overwritten is '\n', so it won't
> return an error.

Good catch. There is actually a history of this code changing back and
forth. However, it's always assumed the last byte to be \n and cut
that off. The original version was this:

char buf[32];
buf_size = min(nbytes, (sizeof(buf) - 1) /* 31 */);
if (copy_from_user(buf, user_buf, buf_size))
        return -EFAULT;
buf[buf_size - 1 /* 30 */] = '\0';

Which expected \n and actually cut off the last copied character. So
without a newline, the window would have been truncated then already.

It also didn't use the full buffer, which Miles fixed in 4adcdcea717c
("sched/psi: Correct overly pessimistic size calculation"):

char buf[32];
buf_size = min(nbytes, (sizeof(buf)) /* 32 */);
if (copy_from_user(buf, user_buf, buf_size))
        return -EFAULT;
buf[buf_size - 1 /* 31 */] = '\0';

but it still cut off that last character, which is either newline or,
welp, the last character of the window spec. Bad, bad.

It also copies the last input byte just to overwrite it again, which
is a bit odd.

> Limiting buf_size to no more than 31 and writing '\0' at the position of
> buf_size can fix this bug.
> 
> Signed-off-by: Yi Tao <escape@linux.alibaba.com>
> ---
>  kernel/sched/psi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 7b4aa5809c0f..5ae336e1c2d8 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -1523,11 +1523,11 @@ static ssize_t psi_write(struct file *file, const char __user *user_buf,
>  	if (!nbytes)
>  		return -EINVAL;
>  
> -	buf_size = min(nbytes, sizeof(buf));
> +	buf_size = min(nbytes, sizeof(buf) - 1);
>  	if (copy_from_user(buf, user_buf, buf_size))
>  		return -EFAULT;
>  
> -	buf[buf_size - 1] = '\0';
> +	buf[buf_size] = '\0';

This looks right. It'll leave a newline in the buffer if present, but
the sscanf() that follows is happy to ignore that.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

I'm thinking we should also CC stable. A truncated window is pretty
difficult to debug, and may not even result in the (ominous) -EINVAL
if the resulting value is still a valid size. It'll quietly poll for
very different parameters than requested.

Cc: stable@vger.kernel.org