Re: [PATCH] 2.4.19 do_adjtimex parameter checking

Re: [PATCH] 2.4.19 do_adjtimex parameter checking

[Ulrich Windl]

On 21 Mar 2002, at 8:40, Paul Gortmaker wrote to me:

> Spotted by Tajthy.Tamas @ datentechnik.hu:
> 
> Adjtimex modes may contain other bits set in addition to
> ADJ_OFFSET_SINGLESHOT bits, and hence tests for strict (in)equality
> are not appropriate - must test for ADJ_OFFSET_SINGLESHOT
> bits set in modes.  Three places in the code where this test
> is made - oddly enough the 3rd is already correct.

Hello, 
masters of the bits,

basically no: adjtimex() is either adjtime() or ntp_adjtime or 
ntp_gettime(). While one could think to set multiple bit combinations, 
it was never intended. At the user level adjtimex() should never have 
existed. This patch would open a new incompatible use of adjtimex() by 
blessing what was illegal before IMHO.

ADJ_OFFSET_SINGLESHOT has be be used alone, specifically also to return 
the correct return value.

Regards,
Ulrich

> 
> 
> --- linux/kernel/time.c~	Thu Feb 28 09:37:32 2002
> +++ linux/kernel/time.c		Thu Mar 21 08:27:49 2002
> @@ -216,7 +216,7 @@
>  		
>  	/* Now we validate the data before disabling interrupts */
>  
> -	if (txc->modes != ADJ_OFFSET_SINGLESHOT && (txc->modes & ADJ_OFFSET))
> +	if (((txc->modes & ADJ_OFFSET_SINGLESHOT) != ADJ_OFFSET_SINGLESHOT) && (txc->modes & ADJ_OFFSET))
>  	  /* adjustment Offset limited to +- .512 seconds */
>  		if (txc->offset <= - MAXPHASE || txc->offset >= MAXPHASE )
>  			return -EINVAL;	
> @@ -275,7 +275,7 @@
>  	    }
>  
>  	    if (txc->modes & ADJ_OFFSET) {	/* values checked earlier */
> -		if (txc->modes == ADJ_OFFSET_SINGLESHOT) {
> +		if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT) {
>  		    /* adjtime() is independent from ntp_adjtime() */
>  		    time_adjust = txc->offset;
>  		}
> 
> 

Re: [PATCH] 2.4.19 do_adjtimex parameter checking

[Paul Gortmaker]

Okay then, the bug still stands, but you want a different fix really.
The user that reported it to me was infact setting multiple bit
combos, which you indicate as taboo (but currently allowed). So 
when multiple bit combos are given, we can either do:

a) return -EINVAL if more than ADJ_OFFSET_SINGLESHOT is set
b) clear/ignore any bits above and beyond ADJ_OFFSET_SINGLESHOT.

As you are a time guru, please indicate which is preferable and I will
bounce Rusty an appropriate patch.

Thanks,
Paul.

Ulrich Windl wrote:
> 
> On 21 Mar 2002, at 8:40, Paul Gortmaker wrote to me:
> 
> > Spotted by Tajthy.Tamas @ datentechnik.hu:
> >
> > Adjtimex modes may contain other bits set in addition to
> > ADJ_OFFSET_SINGLESHOT bits, and hence tests for strict (in)equality
> > are not appropriate - must test for ADJ_OFFSET_SINGLESHOT
> > bits set in modes.  Three places in the code where this test
> > is made - oddly enough the 3rd is already correct.
> 
> Hello,
> masters of the bits,
> 
> basically no: adjtimex() is either adjtime() or ntp_adjtime or
> ntp_gettime(). While one could think to set multiple bit combinations,
> it was never intended. At the user level adjtimex() should never have
> existed. This patch would open a new incompatible use of adjtimex() by
> blessing what was illegal before IMHO.
> 
> ADJ_OFFSET_SINGLESHOT has be be used alone, specifically also to return
> the correct return value.
> 
> Regards,
> Ulrich
> 
> >
> >
> > --- linux/kernel/time.c~      Thu Feb 28 09:37:32 2002
> > +++ linux/kernel/time.c               Thu Mar 21 08:27:49 2002
> > @@ -216,7 +216,7 @@
> >
> >       /* Now we validate the data before disabling interrupts */
> >
> > -     if (txc->modes != ADJ_OFFSET_SINGLESHOT && (txc->modes & ADJ_OFFSET))
> > +     if (((txc->modes & ADJ_OFFSET_SINGLESHOT) != ADJ_OFFSET_SINGLESHOT) && (txc->modes & ADJ_OFFSET))
> >         /* adjustment Offset limited to +- .512 seconds */
> >               if (txc->offset <= - MAXPHASE || txc->offset >= MAXPHASE )
> >                       return -EINVAL;
> > @@ -275,7 +275,7 @@
> >           }
> >
> >           if (txc->modes & ADJ_OFFSET) {      /* values checked earlier */
> > -             if (txc->modes == ADJ_OFFSET_SINGLESHOT) {
> > +             if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT) {
> >                   /* adjtime() is independent from ntp_adjtime() */
> >                   time_adjust = txc->offset;
> >               }
> >
> >

Re: [PATCH] 2.4.19 do_adjtimex parameter checking

[Ulrich Windl]

On 24 Mar 2002, at 5:14, Paul Gortmaker wrote:

> Okay then, the bug still stands, but you want a different fix really.
> The user that reported it to me was infact setting multiple bit
> combos, which you indicate as taboo (but currently allowed). So 
> when multiple bit combos are given, we can either do:
> 
> a) return -EINVAL if more than ADJ_OFFSET_SINGLESHOT is set
> b) clear/ignore any bits above and beyond ADJ_OFFSET_SINGLESHOT.
> 
> As you are a time guru, please indicate which is preferable and I will
> bounce Rusty an appropriate patch.

Thanks for the flowers,

my preference is EINVAL of course. Silently adjusting parameters is bad 
IMHO.

Regards,
Ulrich