[PATCH] sched/psi: Fix the bug where the last character is overwritten

[PATCH] sched/psi: Fix the bug where the last character is overwritten

[Yi Tao]

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.

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';
 
 	seq = file->private_data;
 
-- 
2.32.0.3.g01195cf9f

Re: [PATCH] sched/psi: Fix the bug where the last character is overwritten

[Johannes Weiner]

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

Re: [PATCH] sched/psi: Fix the bug where the last character is overwritten

[Suren Baghdasaryan]

On Wed, Jan 17, 2024 at 9:07 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> 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.

Indeed, nice catch! Thanks!

>
> 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.

Agree.

Acked-by: Suren Baghdasaryan <surenb@google.com>

>
> Cc: stable@vger.kernel.org