Re: [PATCH] tty: change the privilege required for tty operarions

[Greg KH]

On Mon, Jan 15, 2024 at 11:10:29PM +0800, 孟敬姿 wrote:
> 
> 在 2024-1-15 16:35, Greg KH 写道:
> > On Mon, Jan 15, 2024 at 04:24:20PM +0800, Jingzi Meng wrote:
> > > Currently, CAP_SYS_ADMIN is responsible for tty-related functions in
> > > tty_ioctl(): TIOCSTI, TIOCCONS, TIOCVHANGUP. CAP_SYS_ADMIN is already
> > > overloaded, change it to CAP_SYS_TTY_CONFIG for a more fine-grained
> > > and accurate access control.
> > > 
> > > Signed-off-by: Jingzi Meng<mengjingzi@xxxxxxxxx>
> > > ---
> > > 
> > > The userland api affected by this change is the ioctl system call,
> > > especially when the second argument is TIOCSTI, TIOCCONS, TIOCVHANGUP,
> > > which now requires sys_tty_config instead of sys_admin. Tested on Debian
> > > with kernel 6.7.0-rc5.
> > Tested how?  You are changing the permissions of a kernel operation,
> > which is arguably, going to break userspace in lots of interesting ways
> > unless you can prove that this is functionally the same as the existing
> > code.
> > 
> > And not all the world is Debian (although lots of it is, yes.)  But
> > actually running programs that exercise this kernel codepath is going to
> > be the key, did you do that?
> > 
> First of all, this change is not about functionality, only about permissions.

I'm confused, permissions dictate functionality.

If I do not have permissions to do something, the functionality is
suddenly not there.  That's what you are doing here, you are changing
the requirement of previously one capability was required to achive a
function in the kernel, to a different capability.

So it is ALL ABOUT functionality here.

> I wrote 3 testcases which calls ioctl() with TIOCSTI, TIOCCONS, TIOCVHANGUP
> respectively. Then execute them on the origin kernel and patched kernel.
> Running it on both sets of kernels gives the same result. However, through
> the system error message, and the kernel log output I added, I confirmed
> that the relevant functionality under the origin kernel requires sys_admin,
> and under the patched kernel requires sys_tty_config.

I'm referring to existing programs that are not modified to have the new
capability that you are now requiring.  Who is going to do that work?
Where are they?  How have you searched to find them?

> Indeed, it doesn't have much to do with the distro either, I just tested it
> on Debian, and similar tests can be done on other distros.

That's not the issue, the issue is "what existing software is going to
break with this change in permission handling."

That needs to be answered first, and I suggest some research into how we
HAVE TO keep backwards compability and can not break it.

Without some more proof, this type of change can never be accepted, nor
would you want it to be.

good luck!

greg k-h