Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

[Alan Cox]

> Without this ioctl it would have to temporarily become the owner of
> the tty, then call vhangup() and then give it up again.

This is a hack - it's also unfortunately not actually sufficient or
complete which is why we didn't do it years ago. Sorry but if it was easy
it would have been in a long time back !


> +	case TIOCVHANGUP:
> +		if (!capable(CAP_SYS_ADMIN))

Is there any reason for not allowing revocation of a tty that you are
the owner of (ie one you could anyway take ownership of and hangup ?)


> +			return -EPERM;
> +		tty_vhangup(tty);

That raises a few locking questions. From a brief review of the code I
think its directly 1:1 equivalent to allowing a parallel vhangup and
carrier drop which we already do. The tty locks appear to cover this
correctly for the basic stuff although I further review of the ldisc
hangup area from someone with a strong stomache would be good.

You would still need to be very careful how you used it from the admin
side because the parallel

	CPU1					CPU2
	vhangup()				chmod()
	process vhangup
	return
	chown to user1
						chmod to 777
						syscall ends (fd
						revocation takes effect)

	Oops, 0wned

case is not handled by the paths you are using. So to actually do this
you need rather more infrastructure work to ensure the existing calls
have completed before returning.

At that point you've essentially provided revoke() for the tty layer so
it might well be implemented to be called as revoke() as well.

So I don't actually think the patch should be applied in this form,
because it simply adds an interface that will cause problems. Fixed to
return after the revocation is truely complete would be good though.

I'd guess you need to add a counter to the tty f_ops entry/exit points to
know when that occurs, and wake_up the revoke path when ready
(remembering two revokes in parallel shouldn't deadlock! so need
counting too)

In its current form the proposal isn't secure, so NAK, but I'd love to
see it resubmitted with the the other bits done to return at the right
moment.

Alan

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

[Greg KH]

On Fri, Feb 18, 2011 at 09:50:48AM +0000, Alan Cox wrote:
> > Without this ioctl it would have to temporarily become the owner of
> > the tty, then call vhangup() and then give it up again.
> 
> This is a hack - it's also unfortunately not actually sufficient or
> complete which is why we didn't do it years ago. Sorry but if it was easy
> it would have been in a long time back !
> 
> 
> > +	case TIOCVHANGUP:
> > +		if (!capable(CAP_SYS_ADMIN))
> 
> Is there any reason for not allowing revocation of a tty that you are
> the owner of (ie one you could anyway take ownership of and hangup ?)

You could do that already today with the vhangup() syscall, right?

> > +			return -EPERM;
> > +		tty_vhangup(tty);
> 
> That raises a few locking questions. From a brief review of the code I
> think its directly 1:1 equivalent to allowing a parallel vhangup and
> carrier drop which we already do. The tty locks appear to cover this
> correctly for the basic stuff although I further review of the ldisc
> hangup area from someone with a strong stomache would be good.
> 
> You would still need to be very careful how you used it from the admin
> side because the parallel
> 
> 	CPU1					CPU2
> 	vhangup()				chmod()
> 	process vhangup
> 	return
> 	chown to user1
> 						chmod to 777
> 						syscall ends (fd
> 						revocation takes effect)
> 
> 	Oops, 0wned
> 
> case is not handled by the paths you are using. So to actually do this
> you need rather more infrastructure work to ensure the existing calls
> have completed before returning.

But wouldn't this race still happen no matter if vhangup() is in the mix
or not?  I don't see how adding this ioctl changes anything here, what
am I missing?

> At that point you've essentially provided revoke() for the tty layer so
> it might well be implemented to be called as revoke() as well.

It's not a "real" revoke, more like vhangup(file_descriptor) only.
revoke() involves a lot more than just this.

It would be great to have a real revoke() but that seems beyond this
need at the moment.

> So I don't actually think the patch should be applied in this form,
> because it simply adds an interface that will cause problems. Fixed to
> return after the revocation is truely complete would be good though.
> 
> I'd guess you need to add a counter to the tty f_ops entry/exit points to
> know when that occurs, and wake_up the revoke path when ready
> (remembering two revokes in parallel shouldn't deadlock! so need
> counting too)

Again, I'm confused, how does the locking differ from vhangup() today?

thanks,

greg k-h

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

[Alan Cox]

> You could do that already today with the vhangup() syscall, right?

Yes - hence the permission checks are excessive right now.

> > You would still need to be very careful how you used it from the admin
> > side because the parallel
> > 
> > 	CPU1					CPU2
> > 	vhangup()				chmod()
> > 	process vhangup
> > 	return
> > 	chown to user1
> > 						chmod to 777
> > 						syscall ends (fd
> > 						revocation takes effect)
> > 
> > 	Oops, 0wned
> > 
> > case is not handled by the paths you are using. So to actually do this
> > you need rather more infrastructure work to ensure the existing calls
> > have completed before returning.
> 
> But wouldn't this race still happen no matter if vhangup() is in the mix
> or not?  I don't see how adding this ioctl changes anything here, what
> am I missing?

The current users actually handle this - mostly by accident partly by
their nature.

> It's not a "real" revoke, more like vhangup(file_descriptor) only.
> revoke() involves a lot more than just this.

Real revoke is no different. General purpose real revoke for arbitary
objects is a nightmare - but guess what *NO* Unixalike implements that.
 
> > I'd guess you need to add a counter to the tty f_ops entry/exit points to
> > know when that occurs, and wake_up the revoke path when ready
> > (remembering two revokes in parallel shouldn't deadlock! so need
> > counting too)
> 
> Again, I'm confused, how does the locking differ from vhangup() today?

Our vhangup users are not under the delusion that the interface is race
free. Really it wants fixing anyway, but exposing the race with an
inviting new way to screw up isn't good - and the usage model of
vhangup(fd) is such that you can't actually use the ABI for anything
interesting without fixing it.

Also vhangup(fd) with the bug fixed *IS* revoke() so lets stop pretending
it isn't. It's far better to add revoke to the VFS at this point and wire
that to the vhangup + race fixes code that has to be done anyway. In fact
I think it has to be done at that level ultimately to make sure any open
versus hangup races are caught.

It's a new file op to which everyone else (for now) says NULL = no can
do. Really for desktop switching we also want it for some other devices,
so the infrastructure is needed anyway.

revoke is hard, vhangup(fd) is revoke, the rest inevitably follows, but
once it's done then you can add it to a few other cdevs and do all the
nice desktop switching stuff right as well.

It's basically 3 things
- Lennarts bits for vhangup on an fd
- A revoke vfs op
- Counting people in and out of the tty syscalls - which is a hit but its
  not gigabit stuff so won't hurt. Again very easy to implement, and if
  it causes performance corner cases on 4000 CPU boxes smart people can
  fix it later for that. We should do this one anyway
- Ideally dealing with parallel open/revoke paths, which needs the cdev
  code involved

Its not a quick patch - that's why its not happened yet, vhangup(fd)
quickfix Lennart style is unfortunately a useless bodge job which like
most bodge jobs is simply going to spring leaks and need fixing again.

Alan

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

[Lennart Poettering]

On Wed, 23.02.11 00:09, Alan Cox (alan@lxorguk.ukuu.org.uk) wrote:

> It's basically 3 things
> - Lennarts bits for vhangup on an fd

Uh? Me? I didn't write this patch.

(Though I do like to see patch merged and I would use it, and I have
trouble following your logic.)

vhangup() is different from revoke(). vhangup() does weird SIGHUP
handling and stuff, which I think goes way beyond what revoke() would
eventually do. And that different behaviour becomes visible in various
smaller places. e.g. vhangup() results in POLLHUP on the fd, although I
assume that revoke() would more likely result in POLLERR. And there's
more... Let's not pretend this is really the same thing, because it
isn't.

> Its not a quick patch - that's why its not happened yet, vhangup(fd)
> quickfix Lennart style is unfortunately a useless bodge job which like
> most bodge jobs is simply going to spring leaks and need fixing again.

Thanks. If you are trying to insult me, doesn't really work, because I
didn't do this "bodge job". I'll take it as a compliment though that
you say there's a "Lennart style".

Lennart, style icon

-- 
Lennart Poettering - Red Hat, Inc.

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

[Alan Cox]

> vhangup() is different from revoke(). vhangup() does weird SIGHUP
> handling and stuff, which I think goes way beyond what revoke() would
> eventually do. And that different behaviour becomes visible in various

Revoke() will also do this because you remove the controlling process
from the tty. What happens then is mandated by SuS/POSIX.

> > Its not a quick patch - that's why its not happened yet, vhangup(fd)
> > quickfix Lennart style is unfortunately a useless bodge job which like
> > most bodge jobs is simply going to spring leaks and need fixing again.
> 
> Thanks. If you are trying to insult me, doesn't really work,

Not intended as an insult. Believe me I wish a fix like that was viable,
but it's not - which is why its not been in the tree for years.

Alan

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

[Lennart Poettering]

On Tue, 22.02.11 15:15, Greg KH (greg@kroah.com) wrote:

> 
> On Fri, Feb 18, 2011 at 09:50:48AM +0000, Alan Cox wrote:
> > > Without this ioctl it would have to temporarily become the owner of
> > > the tty, then call vhangup() and then give it up again.
> > 
> > This is a hack - it's also unfortunately not actually sufficient or
> > complete which is why we didn't do it years ago. Sorry but if it was easy
> > it would have been in a long time back !
> > 
> > 
> > > +	case TIOCVHANGUP:
> > > +		if (!capable(CAP_SYS_ADMIN))
> > 
> > Is there any reason for not allowing revocation of a tty that you are
> > the owner of (ie one you could anyway take ownership of and hangup ?)
> 
> You could do that already today with the vhangup() syscall, right?

BTW, the reason why this isn't allowed is probably that you really don't
want to allow unprivileged folks to kick privileged users of a
TTY. TTYs can be opened by multiple parties, and stuff such as
/dev/ttyS0 might be used by user logins as well as for logging, and you
don't want to allow users to kick off all loggers just like that.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.