2.4.19 do_adjtimex parameter checking

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.

kernel/time.c renamed from kernel/time.c~ +2 -2

@@ -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;
 		}

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

Quoted 7 lines

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

Quoted 24 lines

kernel/time.c Thu Mar 21 08:27:49 2002 renamed from kernel/time.c~ Thu Feb 28 09:37:32 2002 +2 -2

@@ -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;
              }

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.