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

  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(+)
  

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?

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

thanks
-john
  

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>
  

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
  

On Wed, Nov 23 2022 at 20:53, Arnd Bergmann wrote:
> On Wed, Nov 23, 2022, at 19:54, John Stultz 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
> nevertheless correct, as we should not use any uninitialized
> data in a structure that comes from userspace.

There is no uninitialized data. txc is zeroed at the beginning of that
function.

I agree that it's inconsistent vs. the regular adjtimex(), but as there
is no usage of the txc->tai value coming from user space it's pretty
much cosmetic.

Thanks,

        tglx
  

Jacob!

On Mon, Nov 21 2022 at 00:53, Jacob Macneal wrote:
> Previously, this value was not copied into the output struct.

Previously has no meaning here.

> This is despite all other fields of the corresponding __kernel_timex
> struct being copied to the old_timex32 __user struct in this function.

This is completely backwards. get_old_timex32() copies from the user
supplied old_timex32 struct to the __kernel_timex struct, no?

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

Sure, but the important point is that txc->tai is never used as input
from userspace. It's only ever used as output to userspace, which
explains why this never caused any functional issue.

I'm not against this change per se, but the justification for it really
boils down to:

      Make it consistent with the regular syscall

Thanks,

        tglx
  

Jake,

On Thu, Dec 01 2022 at 10:00, Jake Macneal wrote:
>>> This is despite all other fields of the corresponding __kernel_timex
>>> struct being copied to the old_timex32 __user struct in this function.
>
>> This is completely backwards. get_old_timex32() copies from the user
>> supplied old_timex32 struct to the __kernel_timex struct, no?
>
> You're totally right, I managed to mix up the order right off the bat.
>
>> I'm not against this change per se, but the justification for it really
>> boils down to:
>
>>     Make it consistent with the regular syscall
>

> I agree, that's a better summary. There isn't any effect in the kernel
> now due to get_old_timex32() ignoring the tai value from userspace. So
> this patch would be purely aesthetic, although one might argue that
> copying the userspace tai value into txc is also less likely to lead
> to a bug in the future, were any of the functions do_adjtimex(),
> __do_adjtimex(), or process_adjtimex_modes() to be changed in a way so
> that the userspace tai value became used (I realize this is unlikely).

Right. Unlikely or not, consistency is always a good thing.

> I apologize for any confusion I caused.

No problem. Been there, done that :)

Thanks,

        tglx