Messages in this thread | | | Date | Sun, 19 Mar 2006 00:12:46 +0100 (CET) | Subject | Re: [patch 1/2] Validate itimer timeval from userspace | From | tglx@linutron ... |
| |
Andrew Morton" <akpm@osdl.org> wrote:
>> 1. sys_alarm() >> >> The alarm value 0xFFFFFFFF is valid as the argument to alarm() is an >> unsigned int. So we have to convert this to 0x7FFFFFFF (for 32bit >> machines) because timeval.tv_sec is a signed long. This is done by the >> alarm patch, which is necessary whether we check the sanity of the >> timeval in do_setitimer or not. The current -rc6 kernel sends the alarm >> with the next timer tick, which will break an application which set it >> to something > INT_MAX. >> >> Of course we could do this by the silent conversion of negative values >> in setitimer too. But thats insane as we rely on some broken feature. > > So you're saying that sys_alarm(0xffffffff) needs to behave as > sys_alarm(0x7ffffffff)? > > I guess if we have to do it that way, the risk of breaking anything is > very small. > > What's the 2.4/2.6.13 behaviour of sys_alarm(0xffffffff)?
It gets converted to MAX_SEC_IN_JIFFIES, which depends on HZ, but is definitely <= 0x7fffffff. For HZ = 1000 its 2147483 seconds (~24days), for HZ = 250 its *4 .....
>> 2. setitimer() >> >> An application would have to set value.it_value.tv_sec to a negative >> value to trigger this. Also uninitialized usage of struct timevals can >> cause such behaviour. >> >> I'm not sure, if it is sane to ingore this. > > What does 2.4/2.6.13 do? Let's do that. > > If you're proposing that we depart from previous behaviour by converting > setitimer(0xffffffff) into setitimer(0x7fffffff) then I guess we could > live with that.
Note that this is different to alarm().
alarm(unsigned int seconds); vs. setitimer(struct timeval *value, struct timeval *oldvalue);
So you have to willingly set value->it_value.tv_sec to a negative value or let it randomly uninitialized.
>> I can change the itimer >> validate patch for now to do >> >> if (unlikely(!timeval_valid(v)) >> fixup_timeval(v); >> >> and print an appropriate warning in fixup_timeval() for the time being. >> > > No, we cannot warn - it'll enable unprivileged users to spam the logs. > > One could generate a once-per-reboot warning, I guess. The message should > include the PID and current->comm.
Yeah, I would have limited it to 10 warnings, but I can also do only one.
I make up a patch tomorrow morning.
tglx
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |