[PATCH] tty: replace capable() with file_ns_capable()

[PATCH] tty: replace capable() with file_ns_capable()

[Pranav Tyagi]

The TIOCCONS ioctl currently uses capable(CAP_SYS_ADMIN) to check for
privileges, which validates the current task's credentials. Since this
ioctl acts on an open file descriptor, the check should instead use the
file opener's credentials.

Replace capable() with file_ns_capable() to ensure the capability is
checked against file->f_cred in the correct user namespace. This
prevents unintended privilege escalation and aligns with best practices
for secure ioctl implementations.

Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
Link: https://github.com/KSPP/linux/issues/156
---
 drivers/tty/tty_io.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e2d92cf70eb7..ee0df35d65c3 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -102,6 +102,9 @@
 #include <linux/uaccess.h>
 #include <linux/termios_internal.h>
 #include <linux/fs.h>
+#include <linux/cred.h>
+#include <linux/user_namespace.h>
+#include <linux/capability.h>
 
 #include <linux/kbd_kern.h>
 #include <linux/vt_kern.h>
@@ -2379,7 +2382,7 @@ static int tiocswinsz(struct tty_struct *tty, struct winsize __user *arg)
  */
 static int tioccons(struct file *file)
 {
-	if (!capable(CAP_SYS_ADMIN))
+	if (!file_ns_capable(file, file->f_cred->user_ns, CAP_SYS_ADMIN))
 		return -EPERM;
 	if (file->f_op->write_iter == redirected_tty_write) {
 		struct file *f;
-- 
2.49.0

Re: [PATCH] tty: replace capable() with file_ns_capable()

[Greg KH]

On Sat, Jun 07, 2025 at 07:11:14PM +0530, Pranav Tyagi wrote:
> The TIOCCONS ioctl currently uses capable(CAP_SYS_ADMIN) to check for
> privileges, which validates the current task's credentials. Since this
> ioctl acts on an open file descriptor, the check should instead use the
> file opener's credentials.
> 
> Replace capable() with file_ns_capable() to ensure the capability is
> checked against file->f_cred in the correct user namespace. This
> prevents unintended privilege escalation and aligns with best practices
> for secure ioctl implementations.
> 
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> Link: https://github.com/KSPP/linux/issues/156
> ---
>  drivers/tty/tty_io.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index e2d92cf70eb7..ee0df35d65c3 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -102,6 +102,9 @@
>  #include <linux/uaccess.h>
>  #include <linux/termios_internal.h>
>  #include <linux/fs.h>
> +#include <linux/cred.h>
> +#include <linux/user_namespace.h>
> +#include <linux/capability.h>
>  
>  #include <linux/kbd_kern.h>
>  #include <linux/vt_kern.h>
> @@ -2379,7 +2382,7 @@ static int tiocswinsz(struct tty_struct *tty, struct winsize __user *arg)
>   */
>  static int tioccons(struct file *file)
>  {
> -	if (!capable(CAP_SYS_ADMIN))
> +	if (!file_ns_capable(file, file->f_cred->user_ns, CAP_SYS_ADMIN))

As you now are affecting the user/kernel api here, how was this tested
and are you _SURE_ this is the correct thing to be doing?  Did you audit
all userspace users of this ioctl that you can find (i.e. a Debian code
search) to verify that they can handle this change in behaviour?

I need a lot of assurances before being able to take a change like this,
for obvious reasons.

thanks,

greg k-h

Re: [PATCH] tty: replace capable() with file_ns_capable()

[Pranav Tyagi]

On Sun, Jun 8, 2025 at 3:55 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sat, Jun 07, 2025 at 07:11:14PM +0530, Pranav Tyagi wrote:
> > The TIOCCONS ioctl currently uses capable(CAP_SYS_ADMIN) to check for
> > privileges, which validates the current task's credentials. Since this
> > ioctl acts on an open file descriptor, the check should instead use the
> > file opener's credentials.
> >
> > Replace capable() with file_ns_capable() to ensure the capability is
> > checked against file->f_cred in the correct user namespace. This
> > prevents unintended privilege escalation and aligns with best practices
> > for secure ioctl implementations.
> >
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> > Link: https://github.com/KSPP/linux/issues/156
> > ---
> >  drivers/tty/tty_io.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index e2d92cf70eb7..ee0df35d65c3 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -102,6 +102,9 @@
> >  #include <linux/uaccess.h>
> >  #include <linux/termios_internal.h>
> >  #include <linux/fs.h>
> > +#include <linux/cred.h>
> > +#include <linux/user_namespace.h>
> > +#include <linux/capability.h>
> >
> >  #include <linux/kbd_kern.h>
> >  #include <linux/vt_kern.h>
> > @@ -2379,7 +2382,7 @@ static int tiocswinsz(struct tty_struct *tty, struct winsize __user *arg)
> >   */
> >  static int tioccons(struct file *file)
> >  {
> > -     if (!capable(CAP_SYS_ADMIN))
> > +     if (!file_ns_capable(file, file->f_cred->user_ns, CAP_SYS_ADMIN))
>
> As you now are affecting the user/kernel api here, how was this tested
> and are you _SURE_ this is the correct thing to be doing?  Did you audit
> all userspace users of this ioctl that you can find (i.e. a Debian code
> search) to verify that they can handle this change in behaviour?
>
> I need a lot of assurances before being able to take a change like this,
> for obvious reasons.
>
> thanks,
>
> greg k-h

Hi,

Thank you for the feedback.

I could not find any existing selftests specifically targeting
TIOCCONS. If there are any related ones, I’d appreciate it if you
could point me in the right direction.

Given that, I had to write a standalone functional test to exercise
this ioctl and validate the permission behavior with the proposed
change. I’ll share the test code and output shortly.

I'm also in the process of auditing userspace tools that use
TIOCCONS, using Debian Code Search as suggested. That is taking a
bit of time, but I’ll include the findings in my next update.

In addition, I plan to run integration tests with a few known tools
that rely on TIOCCONS and will report those results as well.

I’d appreciate a bit more time to complete this work and ensure all
bases are covered before resubmitting.

Regards
Pranav Tyagi

Re: [PATCH] tty: replace capable() with file_ns_capable()

[David Laight]

On Sat,  7 Jun 2025 19:11:14 +0530
Pranav Tyagi <pranav.tyagi03@gmail.com> wrote:

> The TIOCCONS ioctl currently uses capable(CAP_SYS_ADMIN) to check for
> privileges, which validates the current task's credentials. Since this
> ioctl acts on an open file descriptor, the check should instead use the
> file opener's credentials.

Is that right?
A terminal will have been opened before the login sequence changed the user id.

The 'best practise' might be to check both!

	David

> 
> Replace capable() with file_ns_capable() to ensure the capability is
> checked against file->f_cred in the correct user namespace. This
> prevents unintended privilege escalation and aligns with best practices
> for secure ioctl implementations.
> 
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> Link: https://github.com/KSPP/linux/issues/156
> ---
>  drivers/tty/tty_io.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index e2d92cf70eb7..ee0df35d65c3 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -102,6 +102,9 @@
>  #include <linux/uaccess.h>
>  #include <linux/termios_internal.h>
>  #include <linux/fs.h>
> +#include <linux/cred.h>
> +#include <linux/user_namespace.h>
> +#include <linux/capability.h>
>  
>  #include <linux/kbd_kern.h>
>  #include <linux/vt_kern.h>
> @@ -2379,7 +2382,7 @@ static int tiocswinsz(struct tty_struct *tty, struct winsize __user *arg)
>   */
>  static int tioccons(struct file *file)
>  {
> -	if (!capable(CAP_SYS_ADMIN))
> +	if (!file_ns_capable(file, file->f_cred->user_ns, CAP_SYS_ADMIN))
>  		return -EPERM;
>  	if (file->f_op->write_iter == redirected_tty_write) {
>  		struct file *f;

Re: [PATCH] tty: replace capable() with file_ns_capable()

[Pranav Tyagi]

On Tue, Jun 17, 2025 at 5:34 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Sat,  7 Jun 2025 19:11:14 +0530
> Pranav Tyagi <pranav.tyagi03@gmail.com> wrote:
>
> > The TIOCCONS ioctl currently uses capable(CAP_SYS_ADMIN) to check for
> > privileges, which validates the current task's credentials. Since this
> > ioctl acts on an open file descriptor, the check should instead use the
> > file opener's credentials.
>
> Is that right?
> A terminal will have been opened before the login sequence changed the user id.
>
> The 'best practise' might be to check both!
>
>         David

Hi,

You're right — I hadn’t fully considered that the terminal is typically
opened before the user ID changes during login. Checking only the file
opener's credentials may miss important security context.

Best practice would indeed be to validate both: ensure that the current
task has sufficient privileges and that the file was opened by an
authorized user.

Thanks for pointing this out — I’ll revise the patch accordingly.

Regards
Pranav Tyagi


>
> >
> > Replace capable() with file_ns_capable() to ensure the capability is
> > checked against file->f_cred in the correct user namespace. This
> > prevents unintended privilege escalation and aligns with best practices
> > for secure ioctl implementations.
> >
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> > Link: https://github.com/KSPP/linux/issues/156
> > ---
> >  drivers/tty/tty_io.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index e2d92cf70eb7..ee0df35d65c3 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -102,6 +102,9 @@
> >  #include <linux/uaccess.h>
> >  #include <linux/termios_internal.h>
> >  #include <linux/fs.h>
> > +#include <linux/cred.h>
> > +#include <linux/user_namespace.h>
> > +#include <linux/capability.h>
> >
> >  #include <linux/kbd_kern.h>
> >  #include <linux/vt_kern.h>
> > @@ -2379,7 +2382,7 @@ static int tiocswinsz(struct tty_struct *tty, struct winsize __user *arg)
> >   */
> >  static int tioccons(struct file *file)
> >  {
> > -     if (!capable(CAP_SYS_ADMIN))
> > +     if (!file_ns_capable(file, file->f_cred->user_ns, CAP_SYS_ADMIN))
> >               return -EPERM;
> >       if (file->f_op->write_iter == redirected_tty_write) {
> >               struct file *f;
>