linux-kernel - Re: [PATCH v2] tty: Add comments for tty-ldisc module loading logic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 15 Dec 2023 18:26:33 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>,
 Zijun Hu <quic_zijuhu@...cinc.com>, gregkh@...uxfoundation.org,
 quic_qiancai@...cinc.com, quic_arandive@...cinc.com,
 quic_saipraka@...cinc.com, quic_eberman@...cinc.com
Cc: linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] tty: Add comments for tty-ldisc module loading logic

On 15. 12. 23, 15:19, Vijaya Krishna Nivarthi wrote:
> Hi,
> 
> 
> On 12/15/2023 7:11 PM, Zijun Hu wrote:
>> Current tty-ldisc module loading logic within tty_ldisc_get()
>> is prone to mislead beginner that the module is able to be loaded
>> by a user without capability CAP_SYS_MODULE, add comments to make
>> the logic easy to undertand.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>> ---
>> Changes in v2:
>> - Remove condition checking changes
>>
>>   drivers/tty/tty_ldisc.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
>> index 3f68e213df1f..34526ffaccbc 100644
>> --- a/drivers/tty/tty_ldisc.c
>> +++ b/drivers/tty/tty_ldisc.c
>> @@ -150,6 +150,10 @@ static struct tty_ldisc *tty_ldisc_get(struct 
>> tty_struct *tty, int disc)
>>        */
>>       ldops = get_ldops(disc);
>>       if (IS_ERR(ldops)) {
>> +        /*
>> +         * Always request tty-ldisc module regardless of user's
>> +         * CAP_SYS_MODULE if autoload is enabled.
>> +         */
> 
> Without much knowledge of this file...
> 
> 
> What the if condition below accomplishes is evident,

After a bit of thinking, sure.

> it probably doesn't require a comment.

I would not add a comment there at all. I would rewrite the code so it 
is obvious to everyone. Like:

static inline bool tty_ldisc_can_autoload(void)
{
   return capable(CAP_SYS_MODULE) || tty_ldisc_autoload;
}

And then:
if (!tty_ldisc_can_autoload())
   return ERR_PTR(-EPERM);

> A more useful comment would be why it does so?

 From an insider, the reason is obvious. But maybe not so much for 
newcomers. Well, one could document the new inline above. Like:
""
We allow loads for capable users or when autoloading is explicitly enabled.
""
or alike...

thanks,
-- 
js
suse labs