Date: Wed, 17 Jan 2024 09:21:30 -0800
From: Suren Baghdasaryan <surenb@...gle.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: Yi Tao <escape@...ux.alibaba.com>, peterz@...radead.org, 
	linux-kernel@...r.kernel.org, Miles Chen <miles.chen@...iatek.com>
Subject: Re: [PATCH] sched/psi: Fix the bug where the last character is overwritten

On Wed, Jan 17, 2024 at 9:07 AM Johannes Weiner <hannes@...xchg.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@...ux.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@...xchg.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@...gle.com>

>
> Cc: stable@...r.kernel.org

Powered by blists - more mailing lists