Re: [PATCH] time: copy tai value (International Atomic Time, in seconds) to output __user struct in get_old_timex32().

From: John Stultz <jstultz@google.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Jacob Macneal <jake.macneal@gmail.com>,
	linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Stephen Boyd <sboyd@kernel.org>
Subject: Re: [PATCH] time: copy tai value (International Atomic Time, in seconds) to output __user struct in get_old_timex32().
Date: Wed, 23 Nov 2022 12:29:05 -0800	[thread overview]
Message-ID: <CANDhNCqzLxsAx=XRBdL554SDZ83vQonnppTQPKgB6=RDhw_XjA@mail.gmail.com> (raw)
In-Reply-To: <7d95f4bb-01a3-44db-af08-275daa9ffb1e@app.fastmail.com>

On Wed, Nov 23, 2022 at 11:53 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Nov 23, 2022, at 19:54, John Stultz wrote:
> > On Sun, Nov 20, 2022 at 9:54 PM Jacob Macneal <jake.macneal@gmail.com> wrote:
> >> --- a/kernel/time/time.c
> >> +++ b/kernel/time/time.c
> >> @@ -311,6 +311,7 @@ int get_old_timex32(struct __kernel_timex *txc, const struct old_timex32 __user
> >>         txc->calcnt = tx32.calcnt;
> >>         txc->errcnt = tx32.errcnt;
> >>         txc->stbcnt = tx32.stbcnt;
> >> +       txc->tai = tx32.tai;
> >>
> >
> > This does seem like something that was overlooked.
> >
> > Arnd: There isn't something more subtle I'm missing here, right?
>
> I agree. Looking at the git history, it seems that the tai field
> was added a long time ago in 153b5d054ac2 ("ntp: support for TAI").
> The commit correctly did the conversion for copying the data out
> of the kernel and did not copy the value in because it wasn't
> needed at the time.
>
> I don't see any user of the tai field that gets copied into
> the kernel, so the bug appears harmless, but Jacob's fix is

Oh, right. There is a quirk of the adjtimex ADJ_TAI interface (added
in 153b5d054ac2) where it for some reason used the constant field
instead of the newly added tai field.
So we never should be using the tai field value from userspace (only
writing it out), which might have been the reason it was not copied
over.

> nevertheless correct, as we should not use any uninitialized
> data in a structure that comes from userspace.

Agreed.

thanks
-john