* Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2024-12-16 15:07 ` [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN Günther Noack
@ 2024-12-16 15:14 ` Greg Kroah-Hartman
2024-12-16 15:17 ` Greg Kroah-Hartman
2024-12-17 9:09 ` [PATCH] " Günther Noack
2 siblings, 0 replies; 25+ messages in thread
From: Greg Kroah-Hartman @ 2024-12-16 15:14 UTC (permalink / raw)
To: Günther Noack
Cc: Jann Horn, Hanno Böck, Jiri Slaby, linux-hardening,
regressions, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2024-12-16 15:07 ` [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN Günther Noack
2024-12-16 15:14 ` Greg Kroah-Hartman
@ 2024-12-16 15:17 ` Greg Kroah-Hartman
2024-12-16 15:42 ` Günther Noack
2024-12-17 9:09 ` [PATCH] " Günther Noack
2 siblings, 1 reply; 25+ messages in thread
From: Greg Kroah-Hartman @ 2024-12-16 15:17 UTC (permalink / raw)
To: Günther Noack
Cc: Jann Horn, Hanno Böck, Jiri Slaby, linux-hardening,
regressions, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2024-12-16 15:17 ` Greg Kroah-Hartman
@ 2024-12-16 15:42 ` Günther Noack
2024-12-21 11:06 ` Günther Noack
0 siblings, 1 reply; 25+ messages in thread
From: Günther Noack @ 2024-12-16 15:42 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jann Horn, Hanno Böck, Jiri Slaby, linux-hardening,
regressions, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2024-12-16 15:42 ` Günther Noack
@ 2024-12-21 11:06 ` Günther Noack
2024-12-21 11:10 ` [PATCH v2] " Günther Noack
2025-01-10 14:21 ` Günther Noack
0 siblings, 2 replies; 25+ messages in thread
From: Günther Noack @ 2024-12-21 11:06 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jann Horn, Hanno Böck, Jiri Slaby, linux-hardening,
regressions, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2024-12-21 11:06 ` Günther Noack
@ 2024-12-21 11:10 ` Günther Noack
2024-12-22 8:37 ` Greg Kroah-Hartman
2025-01-10 14:21 ` Günther Noack
1 sibling, 1 reply; 25+ messages in thread
From: Günther Noack @ 2024-12-21 11:10 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jann Horn, Hanno Böck, Jiri Slaby, linux-hardening,
regressions, linux-kernel, Günther Noack, stable
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).
Cc: stable@vger.kernel.org
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 | 2 --
2 files changed, 14 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..be5564ed8c01 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3345,8 +3345,6 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
switch (type) {
case TIOCL_SETSEL:
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
return set_selection_user(param, tty);
case TIOCL_PASTESEL:
if (!capable(CAP_SYS_ADMIN))
--
2.47.1.613.gc27f4b7a9f-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2024-12-21 11:10 ` [PATCH v2] " Günther Noack
@ 2024-12-22 8:37 ` Greg Kroah-Hartman
0 siblings, 0 replies; 25+ messages in thread
From: Greg Kroah-Hartman @ 2024-12-22 8:37 UTC (permalink / raw)
To: Günther Noack
Cc: Jann Horn, Hanno Böck, Jiri Slaby, linux-hardening,
regressions, linux-kernel, stable
On Sat, Dec 21, 2024 at 11:10:45AM +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).
>
> Cc: stable@vger.kernel.org
> 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 | 2 --
> 2 files changed, 14 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..be5564ed8c01 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3345,8 +3345,6 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
>
> switch (type) {
> case TIOCL_SETSEL:
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> return set_selection_user(param, tty);
> case TIOCL_PASTESEL:
> if (!capable(CAP_SYS_ADMIN))
> --
> 2.47.1.613.gc27f4b7a9f-goog
>
>
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:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/process/submitting-patches.rst for what
needs to be done here to properly describe 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2024-12-21 11:06 ` Günther Noack
2024-12-21 11:10 ` [PATCH v2] " Günther Noack
@ 2025-01-10 14:21 ` Günther Noack
2025-01-10 16:50 ` Kees Cook
2025-01-12 13:14 ` Greg Kroah-Hartman
1 sibling, 2 replies; 25+ messages in thread
From: Günther Noack @ 2025-01-10 14:21 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jann Horn, Hanno Böck, Jiri Slaby, linux-hardening,
regressions, linux-kernel, Günther Noack, stable
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).
Cc: stable@vger.kernel.org
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>
---
Changes in V2:
* Removed comment in vt.c (per Greg's suggestion)
* CC'd stable@
* I *kept* the CAP_SYS_ADMIN check *after* copy_from_user(),
with the reasoning that:
1. I do not see a good alternative to reorder the code here.
We need the data from copy_from_user() in order to know whether
the CAP_SYS_ADMIN check even needs to be performed.
2. A previous get_user() from an adjacent memory region already worked
(making this a very unlikely failure)
I would still appreciate a more formal Tested-by from Hanno (hint, hint) :)
---
drivers/tty/vt/selection.c | 14 ++++++++++++++
drivers/tty/vt/vt.c | 2 --
2 files changed, 14 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..be5564ed8c01 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3345,8 +3345,6 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
switch (type) {
case TIOCL_SETSEL:
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
return set_selection_user(param, tty);
case TIOCL_PASTESEL:
if (!capable(CAP_SYS_ADMIN))
--
2.47.1.613.gc27f4b7a9f-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2025-01-10 14:21 ` Günther Noack
@ 2025-01-10 16:50 ` Kees Cook
2025-02-08 15:18 ` Jared Finder
2025-01-12 13:14 ` Greg Kroah-Hartman
1 sibling, 1 reply; 25+ messages in thread
From: Kees Cook @ 2025-01-10 16:50 UTC (permalink / raw)
To: Günther Noack
Cc: Greg Kroah-Hartman, Jann Horn, Hanno Böck, Jiri Slaby,
linux-hardening, regressions, linux-kernel, stable
On Fri, Jan 10, 2025 at 02:21:22PM +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).
>
> Cc: stable@vger.kernel.org
> 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>
Reviewed-by: Kees Cook <kees@kernel.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2025-01-10 16:50 ` Kees Cook
@ 2025-02-08 15:18 ` Jared Finder
2025-02-08 15:28 ` Greg KH
2025-02-21 0:10 ` Günther Noack
0 siblings, 2 replies; 25+ messages in thread
From: Jared Finder @ 2025-02-08 15:18 UTC (permalink / raw)
To: kees
Cc: gnoack, gregkh, hanno, jannh, jirislaby, linux-hardening,
linux-kernel, regressions, stable
Hi, I'm the original reporter of this regression (noticed because it
impacted GNU Emacs) and I'm wondering if there's any traction on
creating an updated patch? This thread appears to have stalled out. I
haven't seen any reply for three weeks.
-- MJF
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2025-02-08 15:18 ` Jared Finder
@ 2025-02-08 15:28 ` Greg KH
2025-02-08 16:03 ` Jared Finder
2025-02-21 0:10 ` Günther Noack
1 sibling, 1 reply; 25+ messages in thread
From: Greg KH @ 2025-02-08 15:28 UTC (permalink / raw)
To: Jared Finder
Cc: kees, gnoack, hanno, jannh, jirislaby, linux-hardening,
linux-kernel, regressions, stable
On Sat, Feb 08, 2025 at 07:18:22AM -0800, Jared Finder wrote:
> Hi, I'm the original reporter of this regression (noticed because it
> impacted GNU Emacs) and I'm wondering if there's any traction on creating an
> updated patch? This thread appears to have stalled out. I haven't seen any
> reply for three weeks.
It's already in 6.14-rc1 as commit 2f83e38a095f ("tty: Permit some
TIOCL_SETSEL modes without CAP_SYS_ADMIN").
thanks,
greg k-h
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2025-02-08 15:28 ` Greg KH
@ 2025-02-08 16:03 ` Jared Finder
2025-02-09 6:49 ` Greg KH
0 siblings, 1 reply; 25+ messages in thread
From: Jared Finder @ 2025-02-08 16:03 UTC (permalink / raw)
To: Greg KH
Cc: kees, gnoack, hanno, jannh, jirislaby, linux-hardening,
linux-kernel, regressions, stable
On 2025-02-08 07:28, Greg KH wrote:
> On Sat, Feb 08, 2025 at 07:18:22AM -0800, Jared Finder wrote:
>> Hi, I'm the original reporter of this regression (noticed because it
>> impacted GNU Emacs) and I'm wondering if there's any traction on
>> creating an
>> updated patch? This thread appears to have stalled out. I haven't seen
>> any
>> reply for three weeks.
>
> It's already in 6.14-rc1 as commit 2f83e38a095f ("tty: Permit some
> TIOCL_SETSEL modes without CAP_SYS_ADMIN").
Great! Is this expected to get backported to 6.7 through 6.13? I would
like to note the expected resolution correctly in Emacs' bug tracker.
-- MJF
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2025-02-08 16:03 ` Jared Finder
@ 2025-02-09 6:49 ` Greg KH
0 siblings, 0 replies; 25+ messages in thread
From: Greg KH @ 2025-02-09 6:49 UTC (permalink / raw)
To: Jared Finder
Cc: kees, gnoack, hanno, jannh, jirislaby, linux-hardening,
linux-kernel, regressions, stable
On Sat, Feb 08, 2025 at 08:03:27AM -0800, Jared Finder wrote:
> On 2025-02-08 07:28, Greg KH wrote:
> > On Sat, Feb 08, 2025 at 07:18:22AM -0800, Jared Finder wrote:
> > > Hi, I'm the original reporter of this regression (noticed because it
> > > impacted GNU Emacs) and I'm wondering if there's any traction on
> > > creating an
> > > updated patch? This thread appears to have stalled out. I haven't
> > > seen any
> > > reply for three weeks.
> >
> > It's already in 6.14-rc1 as commit 2f83e38a095f ("tty: Permit some
> > TIOCL_SETSEL modes without CAP_SYS_ADMIN").
>
> Great! Is this expected to get backported to 6.7 through 6.13? I would like
> to note the expected resolution correctly in Emacs' bug tracker.
Yes, it should show up in the next round of stable kernel updates later
this week.
But note, it will only show up in the 6.12.y and 6.13.y kernels, as all
others in your range are end-of-life and shouldn't be used by anyone at
this point in time.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2025-02-08 15:18 ` Jared Finder
2025-02-08 15:28 ` Greg KH
@ 2025-02-21 0:10 ` Günther Noack
2025-02-22 21:07 ` Jared Finder
1 sibling, 1 reply; 25+ messages in thread
From: Günther Noack @ 2025-02-21 0:10 UTC (permalink / raw)
To: Jared Finder, hanno
Cc: kees, gnoack, gregkh, jannh, jirislaby, linux-hardening,
linux-kernel, regressions, stable
Hello Jared and Hanno!
On Sat, Feb 08, 2025 at 07:18:22AM -0800, Jared Finder wrote:
> Hi, I'm the original reporter of this regression (noticed because it
> impacted GNU Emacs) and I'm wondering if there's any traction on creating an
> updated patch? This thread appears to have stalled out. I haven't seen any
> reply for three weeks.
>
> -- MJF
Jared, can you please confirm whether Emacs works now with this patch
in the kernel?
I am asking this because I realized that the patch had a bug. We are
erring in the "secure" direction, but not all TIOCL_SELMOUSEREPORT
invocations work without CAP_SYS_ADMIN.
(TIOCL_SELMOUSEREPORT has to be put into the sel_mode field of the
argument struct, and that field looked like an enum to me, but as it
turns out, the TIOCL_SELMOUSEREPORT is 16 and the lower 4 bits of that
integer are used as an additional argument to indicate the mouse
button status and modifier keys. I had overlooked that the
implementation was doing this. As a result, TIOCL_SELMOUSEREPORT
works without CAP_SYS_ADMIN, but only if all four lower bits are 0.)
So, I apologize for the oversight. -- Jared, can you please confirm
whether TIOCL_SELMOUSEREPORT is called directly from Emacs (rather
than from gpm)? I tried to trace it with perf but could not reproduce
a scenario where Emacs called it.
If this specific selection mode is not needed by Emacs, I think *the
best thing would be to keep it guarded by CAP_SYS_ADMIN, after all*.
As it turns out, the following scenario is possible:
* A root shell invokes malicious program P and changes its UID to a
less privileged user, but it passes a FD to the TTY. (Instead of
UID switch, it can also be a sandboxed program or another way of
lowering privileges.)
* Program P enables mouse tracking mode by writing "\033[?1000h".
* Program P sends IOCTL TIOCLINUX with TIOCL_SETSEL with
TIOCL_SELMOUSEREPORT and passes suitable mouse coordinate and button
press arguments. As a response, the terminal writes the escape
sequence \033[MBXY, where B, X and Y are bytes indicating the button
press mask and the 1-based mouse X and Y coordinates, all added to
0x20 (space).
It is an obscure scenario and probably requires a console with a
character width and height above 256 (to control the full byte range),
but it seems that this can in principle be used to simulate short
keyboard inputs to programs (like bash) that are not expecting this
old mouse protocol. - This sort of keypress-simulation is exactly why
we created the CAP_SYS_ADMIN restriction for TIOCL_SETSEL in the first
place.
For background on this mouse tracking mechanism, I had to read up on
it myself, but found the following two links helpful:
https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Normal-tracking-mode
https://www.ibm.com/docs/en/aix/7.2?topic=x-xterm-command#xterm__mouse
(Remark on the side, the GPM client library normally gets its mouse
coordinates through a Unix Domain socket from the GPM daemon. It has
support for this xterm emulation mode as well, but it is only enabled
if $TERM starts with "xterm".)
In summary:
If it is not absolutely needed, I think it would be better to not
permit access to TIOCL_SELMOUSEREPORT after all. It does not let
attackers simulate keypresses quite as easily as the other features,
but it does let them send such input with more limited control, and it
seems like an unnecessary risk if the feature is not needed by
anything but mouse daemons running as root, such as GPM and
Consolation.
Does this seem reasonable? (Hanno, do you agree with this
assessment?) I am by no means an expert in this terminal-mouse
interaction, I am happy to stand corrected if I am wrong here.
–Günther
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2025-02-21 0:10 ` Günther Noack
@ 2025-02-22 21:07 ` Jared Finder
0 siblings, 0 replies; 25+ messages in thread
From: Jared Finder @ 2025-02-22 21:07 UTC (permalink / raw)
To: Günther Noack
Cc: hanno, kees, gnoack, gregkh, jannh, jirislaby, linux-hardening,
linux-kernel, regressions, stable
On 2025-02-20 16:10, Günther Noack wrote:
>
> Jared, can you please confirm whether Emacs works now with this patch
> in the kernel?
>
> I am asking this because I realized that the patch had a bug. We are
> erring in the "secure" direction, but not all TIOCL_SELMOUSEREPORT
> invocations work without CAP_SYS_ADMIN.
I confirmed that Emacs worked fine with 6.14-rc1. My understanding is
that the Emacs process relies only on TIOCL_SELPOINTER which it needs to
do to draw the mouse pointer after Emacs' redisplay. It's fine for
TIOCL_SELMOUSEREPORT to not work in an unpriviliged Emacs.
> If this specific selection mode is not needed by Emacs, I think *the
> best thing would be to keep it guarded by CAP_SYS_ADMIN, after all*.
This sounds good to me.
Reading over a documentation proposal for TIOCL_SELMOUSEREPORT
(https://lkml.org/lkml/2020/7/6/249), I can not imagine how a userspace
program that was not acting as the mouse daemon could successfully use
SELMOUSEREPORT as the mouse daemon will be fighting with it. Any
legitimate setting of mouse state (for example, setting the mouse x/y
coordinate) would need to be done with the mouse daemon in the loop, in
which case the mouse daemon might as well send the message itself.
-- MJF
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2025-01-10 14:21 ` Günther Noack
2025-01-10 16:50 ` Kees Cook
@ 2025-01-12 13:14 ` Greg Kroah-Hartman
1 sibling, 0 replies; 25+ messages in thread
From: Greg Kroah-Hartman @ 2025-01-12 13:14 UTC (permalink / raw)
To: Günther Noack
Cc: Jann Horn, Hanno Böck, Jiri Slaby, linux-hardening,
regressions, linux-kernel, stable
On Fri, Jan 10, 2025 at 02:21:22PM +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).
>
> Cc: stable@vger.kernel.org
> 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>
> ---
> Changes in V2:
>
> * Removed comment in vt.c (per Greg's suggestion)
>
> * CC'd stable@
>
> * I *kept* the CAP_SYS_ADMIN check *after* copy_from_user(),
> with the reasoning that:
>
> 1. I do not see a good alternative to reorder the code here.
> We need the data from copy_from_user() in order to know whether
> the CAP_SYS_ADMIN check even needs to be performed.
> 2. A previous get_user() from an adjacent memory region already worked
> (making this a very unlikely failure)
>
> I would still appreciate a more formal Tested-by from Hanno (hint, hint) :)
This really is v3, as you sent a v2 last year, right?
b4 got really confused, but I think I figured it out. Whenever you
resend, bump the version number please, otherwise it causes problems.
Remember, some of use deal with thousands of patches a week...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
2024-12-16 15:07 ` [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN Günther Noack
2024-12-16 15:14 ` Greg Kroah-Hartman
2024-12-16 15:17 ` Greg Kroah-Hartman
@ 2024-12-17 9:09 ` Günther Noack
2 siblings, 0 replies; 25+ messages in thread
From: Günther Noack @ 2024-12-17 9:09 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jann Horn, Hanno Böck, Jiri Slaby, linux-hardening,
regressions, linux-kernel
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
>
^ permalink raw reply [flat|nested] 25+ messages in thread