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

From: Günther Noack
Date: Mon Dec 16 2024 - 10:43:48 EST


Hello!

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@xxxxxxxxxx
> > Fixes: 8d1b43f6a6df ("tty: Restrict access to TIOCLINUX' copy-and-paste subcommands")
> > Signed-off-by: Günther Noack <gnoack@xxxxxxxxxx>
> > ---
> > 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?


As background, we have only introduced the CAP_SYS_ADMIN check recently in
8d1b43f6a6df ("tty: Restrict access to TIOCLINUX' copy-and-paste subcommands")
The patch landed in Linux 6.7 and was discussed in
https://lore.kernel.org/all/20230828164117.3608812-1-gnoack@xxxxxxxxxx/.

I suspect that existing callers of the TIOCL_SETSEL IOCTL are probably all
written at a time before the capability check existed. ((a) These IOCTLs are
used for the console mouse support, and (b) these "modes" for the TIOCL_SETSEL
subcode for the TIOCLINUX IOCTL are not documented in the man page either.)

>
> > +
> > return set_selection_kernel(&v, tty);
> > }
> >
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 96842ce817af..ed65b3b80fbd 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -3345,8 +3345,7 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
> >
> > switch (type) {
> > case TIOCL_SETSEL:
> > - if (!capable(CAP_SYS_ADMIN))
> > - return -EPERM;
> > + /* CAP_SYS_ADMIN check happens in set_selection_user(). */
>
> No need to mention this here, it's obvious in the function itself.

OK, thanks. I will remove it in the next version.

—Günther