| Message ID | 20241216150722.4079213-2-gnoack@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN | expand |
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(-) Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - Your patch breaks the build. - Your patch contains warnings and/or errors noticed by the scripts/checkpatch.pl tool. - You have marked a patch with a "Fixes:" tag for a commit that is in an older released kernel, yet you do not have a cc: stable line in the signed-off-by area at all, which means that the patch will not be applied to any older kernel releases. To properly fix this, please follow the documented rules in the Documentation/process/stable-kernel-rules.rst file for how to resolve this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
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? > + > 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. thanks, greg k-h
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@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? 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@google.com/. 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
FYI, I have tested this locally - it makes the mouse curser show up again in Emacs. (I do also believe that is it fine security-wise, but will also very happily add a Reviewed-By or Tested-By from Hanno or others in the next iteration :)) —Günther 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; > + } > + > 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(). */ > return set_selection_user(param, tty); > case TIOCL_PASTESEL: > if (!capable(CAP_SYS_ADMIN)) > -- > 2.47.1.613.gc27f4b7a9f-goog >
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
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; + } + 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(). */ return set_selection_user(param, tty); case TIOCL_PASTESEL: if (!capable(CAP_SYS_ADMIN))
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(-)