[9fans] devproc noteid changing for none

[9fans] devproc noteid changing for none

[cinap_lenrek]

one can change the note group of a process with devproc
by writing noteid file.

	case Qnoteid:
		id = atoi(a);
		if(id == p->pid) {
			p->noteid = id;
			break;
		}
		t = proctab(0);
		for(et = t+conf.nproc; t < et; t++) {
			if(t->state == Dead)
				continue;
			if(id == t->noteid) {
				if(strcmp(p->user, t->user) != 0)
					error(Eperm);
				p->noteid = id;
				break;
			}
		}
		if(p->noteid != id)
			error(Ebadarg);
		break;

the strcmp() check in that loop isnt enougth when the
user doing the write is "none" as this would allow him
to change the noteid of its process to another "none"
session and and then kill it. like for example to one
of the aux/listen procs.

the rules for "none" user is that he cant open other
processes running as "none" except its own calling
proc.

http://code.google.com/p/plan9front/source/detail?r=118280a79161c8cf42164bcc9458af7650652f91

--
cinap

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

On Thu Jan  2 16:19:12 EST 2014, cinap_lenrek@felloff.net wrote:
> one can change the note group of a process with devproc
> by writing noteid file.
>
> 	case Qnoteid:
> 		id = atoi(a);
> 		if(id == p->pid) {
> 			p->noteid = id;
> 			break;
> 		}
> 		t = proctab(0);
> 		for(et = t+conf.nproc; t < et; t++) {
> 			if(t->state == Dead)
> 				continue;
> 			if(id == t->noteid) {
> 				if(strcmp(p->user, t->user) != 0)
> 					error(Eperm);
> 				p->noteid = id;
> 				break;
> 			}
> 		}
> 		if(p->noteid != id)
> 			error(Ebadarg);
> 		break;

procopen() does nonone for Qnoteid.  isn't that enough?

- erik

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

no, its not. because "none" can indeed open its own noteid file.
nonone makes sure that it cannot open the noteid file of a process
*different* from up when up->user == "none".

--
cinap

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

a process running as "none" can only access its own (calling) process.

but noteid write allows it to change the noteid of its own process to
a nother group (also running as none) which allows it to send notes
to that group.

this has to be prevented.

--
cinap

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

On Thu Jan  2 16:33:33 EST 2014, cinap_lenrek@felloff.net wrote:
> a process running as "none" can only access its own (calling) process.
>
> but noteid write allows it to change the noteid of its own process to
> a nother group (also running as none) which allows it to send notes
> to that group.
>
> this has to be prevented.

; cd /proc/$pid; pwd
/proc/75189
; cat noteid
      76810 ;
; auth/none
; cd /proc/$pid; pwd
/proc/75192
; cat noteid
cat: can't open noteid: 'noteid' permission denied

- erik

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

thats because cat tries to open the noteid file of
your rc shell (running as none). and not the noteid
file of its own process (cat).

static void
nonone(Proc *p)
{
	if(p == up)
		return;
	if(strcmp(up->user, "none") != 0)
		return;
	if(iseve())
		return;
	error(Eperm);
}

nonone() is always ok for p == up. in your case, p != up
so it fails (as expected).

--
cinap

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

> ; cd /proc/$pid; pwd
> /proc/75189
> ; cat noteid
>       76810 ;
> ; auth/none
> ; cd /proc/$pid; pwd
> /proc/75192
> ; cat noteid
> cat: can't open noteid: 'noteid' permission denied

never mind.  different process.  how did you find this, anyway?

- erik

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

i was investigating all callers of postnote() for bugs that could
lead to spurious notes like the alarm race i described before. btw
this has been fixed too. the key is to recheck p->alarm while holding
p->debug qlock. once you have it, the process cannot exit under you.

--
cinap

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

On Thu Jan  2 16:55:59 EST 2014, cinap_lenrek@felloff.net wrote:
> i was investigating all callers of postnote() for bugs that could
> lead to spurious notes like the alarm race i described before. btw
> this has been fixed too. the key is to recheck p->alarm while holding
> p->debug qlock. once you have it, the process cannot exit under you.

cool.  if that's the protocol, doesn't the debug lock need to be held whenever
up->alarm is modified?

- erik

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

On Thu Jan  2 17:32:08 EST 2014, quanstro@quanstro.net wrote:
> On Thu Jan  2 16:55:59 EST 2014, cinap_lenrek@felloff.net wrote:
> > i was investigating all callers of postnote() for bugs that could
> > lead to spurious notes like the alarm race i described before. btw
> > this has been fixed too. the key is to recheck p->alarm while holding
> > p->debug qlock. once you have it, the process cannot exit under you.
>
> cool.  if that's the protocol, doesn't the debug lock need to be held whenever
> up->alarm is modified?

checkalarms will hang during rollover.  suppose sys->ticks = 1<<31,
then (long)((1<<31) - 0) = -2147483648.  this will stay negative for a
2147483648/HZ seconds.

i think this is necessary:

void
checkalarms(void)
{
	Proc *p;
	ulong now;

	p = alarms.head;
	now = sys->ticks;

	if(p != nil)
	if(p->alarm == 0 || (long)(now - p->alarm) >= 0)
		wakeup(&alarmr);
}

- erik

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

i know. i fixed that too. :)

--
cinap

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

no. the debug qlock is so the process wont exit. once you hold it,
the process might be running, might be in the process of exiting
might have been already exited or might have been reused.

the key here is that while you hold it. it wont make it to the
process freelist when not already there. it wont change
from "exiting -> exited". but it wont prevent it from being
reused when it was exited already... but...

pexit() zeros p->alarm. and p->alarm can not be set to non-zero
value again while we hold the alarms qlock in the kproc!

so this actually works.

--
cinap

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

On Thu Jan  2 17:54:50 EST 2014, cinap_lenrek@felloff.net wrote:
> no. the debug qlock is so the process wont exit. once you hold it,
> the process might be running, might be in the process of exiting
> might have been already exited or might have been reused.
>
> the key here is that while you hold it. it wont make it to the
> process freelist when not already there. it wont change
> from "exiting -> exited". but it wont prevent it from being
> reused when it was exited already... but...
>
> pexit() zeros p->alarm. and p->alarm can not be set to non-zero
> value again while we hold the alarms qlock in the kproc!
>
> so this actually works.
>

the code says you can set alarm = 0 without holding the alarms lock.

ulong
procalarm(ulong time)
{
	Proc **l, *f;
	ulong when, old;

	if(up->alarm)
		old = tk2ms(up->alarm - sys->ticks);
	else
		old = 0;
	if(time == 0) {
>>		up->alarm = 0;
>>		return old;

- erik

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

exactly. but you cannot set it to non-zero value again. which is
what we are interested in to prevent sending the note when the
process has been reused.

--
cinap

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

On Thu Jan  2 18:02:33 EST 2014, cinap_lenrek@felloff.net wrote:
> exactly. but you cannot set it to non-zero value again. which is
> what we are interested in to prevent sending the note when the
> process has been reused.

it's also important for alarm(0) to complete or not complete.  without
holding the lock, alarm(0) can return 0 and then deliver a note!

- erik

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

said other. it is ok to send the note when p->alarms is non-zero
while holding the p->debug lock. it might just change to zero
after our check but this is ok. we will send the note but pexit()
will not make it to put the process on the freelist until we
release the debug qlock.

--
cinap

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

and yes. you can get follow up alarm on alarm(0); when the alarm had
already triggered. this is unavoidable. say, the alarm could'v posted
the note just before you called alarm(0); as well, but the note was
not delivered yet. once the alarm() syscall returns to userspace, it
would pickup the posted note and call the note handler.

--
cinap

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

that said, just holding alarms qlock while setting up->alarm = 0
is not enougth when you want to guarantee that no alarms will be
delivered when the alarm(0) syscall returns. you would also need to
remove a potentially posted alarm note from your note array and
reset up->notepending (all under debug qlock of course).

--
cinap

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

On Thu Jan  2 18:18:54 EST 2014, cinap_lenrek@felloff.net wrote:
> that said, just holding alarms qlock while setting up->alarm = 0
> is not enougth when you want to guarantee that no alarms will be
> delivered when the alarm(0) syscall returns. you would also need to
> remove a potentially posted alarm note from your note array and
> reset up->notepending (all under debug qlock of course).

that's true.  up->nnote is not atomicly incremented.  alarm is an
insideous little monster.  it looks like a simple interface, but it is not.

- erik

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

i think the list insertion code needs a single-read
test that f->alarm != 0. to prevent the 0 from
acting like a fencepost.  e.g. trying to insert -10 into
list -40 -30 0 -20.

	if(alarms.head) {
		l = &alarms.head;
		for(f = *l; f; f = f->palarm) {
>>			fw = f->alarm;
>>			if(fw != 0 && (long)(fw - when) >= 0) {
				up->palarm = f;
				*l = up;
				goto done;
			}
			l = &f->palarm;
		}
		*l = up;
	}


- erik

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

ah, yes.

--
cinap

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

On Thu Jan  2 19:33:37 EST 2014, cinap_lenrek@felloff.net wrote:
> ah, yes.

i ran some tests on this version, and it seems to work fine across
tick timer wraparound.  this was done by artificially setting sys->ticks negative
on boot.  also, there was some cheezy added code to the timer check loop
to make sure that all the timers were in order (mod zeros).

that being said, let's hope this really is the last of the gremlins
in alarm.

- erik

Re: [9fans] devproc noteid changing for none

[cinap_lenrek]

good job.

--
cinap

Re: [9fans] devproc noteid changing for none

[erik quanstrom]

On Thu Jan  2 20:24:29 EST 2014, cinap_lenrek@felloff.net wrote:
> good job.

thanks you you!

- erik