[PATCH v1] tty: Fix a security issue related to tty-ldisc module loading

[PATCH v1] tty: Fix a security issue related to tty-ldisc module loading

[Zijun Hu]

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.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 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)
 			return ERR_PTR(-EPERM);
 		request_module("tty-ldisc-%d", disc);
 		ldops = get_ldops(disc);
-- 
The Qualcomm Innovation Center

Re: [PATCH v1] tty: Fix a security issue related to tty-ldisc module loading

[Greg KH]

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?

> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  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?

Remember if tty_ldisc_autoload is enabled, then any user can auto-load a
tty ldisc, permissions are not needed.

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?

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?

What am I missing here?

confused,

greg "boolean logic is hard" k-h

Re: [PATCH v1] tty: Fix a security issue related to tty-ldisc module loading

[quic_zijuhu]

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@quicinc.com>
>> ---
>>  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

Re: [PATCH v1] tty: Fix a security issue related to tty-ldisc module loading

[Greg KH]

On Fri, Dec 15, 2023 at 05:32:52PM +0800, quic_zijuhu wrote:
> 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.

Yes, that is as-intended, why are you trying to break existing
functionality that has been present for forever?

> current logical is weird and it confuse me as a tty driver beginner since the intuitive checking is shown by my change.

It might be confusing, but it is correct.  You have to justify changing
existing functionality a lot, especially for user-visable stuff like
this.

And to say it is a "security issue" is not correct, it is this way by
design, please work to understand history before attempting to change it
for no documented reason.  Did you read the config option that helps
control this functionality?  Did the help text there not explain it
properly?  If so, please provide additional documentation where needed.

I suggest working with others at your company that have more experience
before submitting changes like this in the future, as they should be
able to help you out better instead of relying on the community to do
so.

thanks,

greg k-h

Re: [PATCH v1] tty: Fix a security issue related to tty-ldisc module loading

[quic_zijuhu]

On 12/15/2023 6:38 PM, Greg KH wrote:
> On Fri, Dec 15, 2023 at 05:32:52PM +0800, quic_zijuhu wrote:
>> 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.
> 
> Yes, that is as-intended, why are you trying to break existing
> functionality that has been present for forever?
> 
i understood current design by looking at historical commit and agree that current design is okay.

>> current logical is weird and it confuse me as a tty driver beginner since the intuitive checking is shown by my change.
> 
> It might be confusing, but it is correct.  You have to justify changing
> existing functionality a lot, especially for user-visable stuff like
> this.
> 
i will add more comments and optimize checking logical but remain current logic in order to make it easy to understand
> And to say it is a "security issue" is not correct, it is this way by
> design, please work to understand history before attempting to change it
> for no documented reason.  Did you read the config option that helps
> control this functionality?  Did the help text there not explain it
> properly?  If so, please provide additional documentation where needed.
> 
make sense.
> I suggest working with others at your company that have more experience
> before submitting changes like this in the future, as they should be
> able to help you out better instead of relying on the community to do
> so.
> 
> thanks,
> 
> greg k-h