On 12/15/2023 4:43 PM, Greg KH wrote:
> On Fri, Dec 15, 2023 at 04:28:53PM +0800, Zijun Hu wrote:
>> Function tty_ldisc_get() has a simple logical error and may cause tty-ldisc
>> module to be loaded by a user without CAP_SYS_MODULE, this security issue
>> is fixed by correcting the logical error.
>
> What specific security issue are you referring to here?
module tty-ldisc is able to be loaded by a user who don't have relevant permission CAP_SYS_MODULE to load module.
current logical is weird and it confuse me as a tty driver beginner since the intuitive checking is shown by my change.
when you want to load a module, check by the following sequences:
1) if you have relevant permission CAP_SYS_MODULE to do it. do NOT do it and return if you don't have permission.
2) then check if we need to do it based on configuration tty_ldisc_autoload, if not configure to do it, do NOT it and return.
3) then do it if PASS previous 2 checks.
>
>> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>> ---
>> drivers/tty/tty_ldisc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
>> index 3f68e213df1f..b490c0adf00f 100644
>> --- a/drivers/tty/tty_ldisc.c
>> +++ b/drivers/tty/tty_ldisc.c
>> @@ -150,7 +150,7 @@ static struct tty_ldisc *tty_ldisc_get(struct tty_struct *tty, int disc)
>> */
>> ldops = get_ldops(disc);
>> if (IS_ERR(ldops)) {
>> - if (!capable(CAP_SYS_MODULE) && !tty_ldisc_autoload)
>> + if (!capable(CAP_SYS_MODULE) || !tty_ldisc_autoload)
>
> I'm missing something, why change this?
>
make it follow normal checking logic as mentioned for your 1st question.
> Remember if tty_ldisc_autoload is enabled, then any user can auto-load a
> tty ldisc, permissions are not needed.
>
it so, it is good to add some comments or optimize checking logic and make it easy to understand.
> as it's confusing to read, let me break this down to see if the original
> code is correct or not:
> If you do NOT have CAP_SYS_MODULE AND you do NOT have
> tty_ldisc_autoload enabled, then the kernel will NOT call
> request_module
>
> If you do have CAP_SYS_MODULE enabled then the kernel will call
> request_module()
>
> If you do have tty_ldisc_autoload enabled, then you can autoload
> a module.
>
> Is this not the correct functionality?
> not sure what is the expected functionality when wrote current checking logic.
it seems kernel checks here for a user (perhaps user space process) should load tty-ldisc module.
what are expected actions when wrote current checkings.what is expected actions for below combine conditions?
//user do NOT have permission to load module autoloaded disabled. what is expected action? load or unload the module.
CAP_SYS_MODULE = no, tty_ldisc_autoload = no, ?
CAP_SYS_MODULE = no, tty_ldisc_autoload = yes, ?
CAP_SYS_MODULE = yes, tty_ldisc_autoload = no, ?
CAP_SYS_MODULE = yes, tty_ldisc_autoload = yes, ?
> You are changing this to:
> If you do NOT have CAP_SYS_MODULE enabled, then no matter what,
> do NOT call request_module()
>
> If you do NOT have tty_ldisc_autoload enabled, then no matter
> what, do NOT call request_module()
>
> Are you sure that's what you want to change this to?
>
yes.
> What am I missing here?
>
nothing.
> confused,
>
> greg "boolean logic is hard" k-h
