Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

["Günther Noack" <gnoack@google.com>]

Hello Greg, Hanno and everyone else!

On Mon, Dec 16, 2024 at 04:42:50PM +0100, Günther Noack wrote:
> On Mon, Dec 16, 2024 at 04:17:15PM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Dec 16, 2024 at 03:07:23PM +0000, Günther Noack wrote:
> > > With this, processes without CAP_SYS_ADMIN are able to use TIOCLINUX with
> > > subcode TIOCL_SETSEL, in the selection modes TIOCL_SETPOINTER,
> > > TIOCL_SELCLEAR and TIOCL_SELMOUSEREPORT.
> > > 
> > > TIOCL_SETSEL was previously changed to require CAP_SYS_ADMIN, as this IOCTL
> > > let callers change the selection buffer and could be used to simulate
> > > keypresses.  These three TIOCL_SETSEL selection modes, however, are safe to
> > > use, as they do not modify the selection buffer.
> > > 
> > > This fixes a mouse support regression that affected Emacs (invisible mouse
> > > cursor).
> > > 
> > > Link: https://lore.kernel.org/r/ee3ec63269b43b34e1c90dd8c9743bf8@finder.org
> > > Fixes: 8d1b43f6a6df ("tty: Restrict access to TIOCLINUX' copy-and-paste subcommands")
> > > Signed-off-by: Günther Noack <gnoack@google.com>
> > > ---
> > >  drivers/tty/vt/selection.c | 14 ++++++++++++++
> > >  drivers/tty/vt/vt.c        |  3 +--
> > >  2 files changed, 15 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
> > > index 564341f1a74f..0bd6544e30a6 100644
> > > --- a/drivers/tty/vt/selection.c
> > > +++ b/drivers/tty/vt/selection.c
> > > @@ -192,6 +192,20 @@ int set_selection_user(const struct tiocl_selection __user *sel,
> > >  	if (copy_from_user(&v, sel, sizeof(*sel)))
> > >  		return -EFAULT;
> > >  
> > > +	/*
> > > +	 * TIOCL_SELCLEAR, TIOCL_SELPOINTER and TIOCL_SELMOUSEREPORT are OK to
> > > +	 * use without CAP_SYS_ADMIN as they do not modify the selection.
> > > +	 */
> > > +	switch (v.sel_mode) {
> > > +	case TIOCL_SELCLEAR:
> > > +	case TIOCL_SELPOINTER:
> > > +	case TIOCL_SELMOUSEREPORT:
> > > +		break;
> > > +	default:
> > > +		if (!capable(CAP_SYS_ADMIN))
> > > +			return -EPERM;
> > > +	}
> > 
> > Shouldn't you check this _before_ doing the copy_from_user() to emulate
> > the previous logic properly?
> 
> You are right, I believe this can technically return a different error code.
> 
> There is a data dependency between the two though - the capability check should
> only happen for certain values of v.sel_mode, and v is only populated by
> copy_from_user().
> 
> As far as I can tell, this only makes a difference in scenarios where both
> copy_from_user() and the capability check would fail.
> 
> Do you think we have to emulate it down to this level of detail?

Another observation which makes it clearer that copy_from_user() failing here
should really not happen in practice:

The 'argp' pointer that gets passed to ioctl(2) here is a pointer to a region in
memory whose first byte is a TIOCLINUX subcommand, and at the time we do the
copy_from_user(), this subcommand byte has already been successfully copied over
from user space.  The region that we are now copying with copy_from_user() here
is directly after this byte, and should under normal circumstances work as well
-- I believe that callers would have to construct a specifically crafted ioctl()
so that the first copy-from-userspace works and the second one fails (referring
to a byte just before a page boundary, or something like that...?)

I'll send a V2 of this patch with the comment removed, the 'Cc: stable' added,
but where the CAP_SYS_ADMIN check is still done after copy_from_user(), with the
reasoning that:

 1. A previous get_user() from an adjacent memory region already worked
    (making this a very unlikely failure)
 2. I do not see a good alternative to reorder the code here, because we need
    the data from copy_from_user() in order to know whether the CAP_SYS_ADMIN
    check even needs to be performed.

Hanno: If you are OK with this patch, I'd still appreciate a more formal
"Reviewed-By" or "Tested-By" from you. :)

Thanks, and Happy Holidays!
—Günther