Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-1115-g8b801eadce-fm-20221102.001-g8b801ead
Mime-Version: 1.0
Message-Id: <7d95f4bb-01a3-44db-af08-275daa9ffb1e@app.fastmail.com>
In-Reply-To: <CANDhNCqCYkv7Ja4RT2_DEPh2zFFL+SD82wru_FC6dupCo6_4RA@mail.gmail.com>
References: <20221121055345.111567-1-jake.macneal@gmail.com>
 <CANDhNCqCYkv7Ja4RT2_DEPh2zFFL+SD82wru_FC6dupCo6_4RA@mail.gmail.com>
Date:   Wed, 23 Nov 2022 20:53:14 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "John Stultz" <jstultz@google.com>,
        "Jacob Macneal" <jake.macneal@gmail.com>
Cc:     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().
Content-Type: text/plain
Precedence: bulk

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:
>>
>> Previously, this value was not copied into the output struct. This is
>> despite all other fields of the corresponding __kernel_timex struct being
>> copied to the old_timex32 __user struct in this function.
>>
>> Additionally, the matching function put_old_timex32() expects a tai value
>> to be supplied, and copies it appropriately. It would appear to be a
>> mistake that this value was never copied over in get_old_timex32().
>>
>> Signed-off-by: Jacob Macneal <jake.macneal@gmail.com>
>> ---
>>  kernel/time/time.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/time/time.c b/kernel/time/time.c
>> index 526257b3727c..7da9951b033a 100644
>> --- 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
nevertheless correct, as we should not use any uninitialized
data in a structure that comes from userspace.

> Otherwise:
>   Acked-by: John Stultz <jstultz@google.com>
>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>