linux-kernel - Re: [PATCH v2] tty: tty_io: remove hung_up_tty

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 30 May 2023 20:57:42 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Dmitry Vyukov <dvyukov@...gle.com>,
        syzbot <syzbot+b7c3ba8cdc2f6cf83c21@...kaller.appspotmail.com>,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        Nathan Chancellor <nathan@...nel.org>,
        Arnd Bergmann <arnd@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Jiri Slaby <jirislaby@...nel.org>
Subject: Re: [PATCH v2] tty: tty_io: remove hung_up_tty_fops

On 2023/05/30 19:44, Greg Kroah-Hartman wrote:
> On Sun, May 14, 2023 at 10:02:26AM +0900, Tetsuo Handa wrote:
>> If we care about only NULL pointer dereference, implementing missing
>> callbacks to hung_up_tty_fops is fine. But if we also care about KCSAN
>> reports, we will need to wrap all filp->f_op usages which are reachable
>> via tty_fops callbacks using data_race().
> 
> I'm missing something here.  Why would KCSAN report problems if we
> implement the needed callbacks in hung_up_tty_fops?  And what reports
> would they be?

Unlike atomic operations such as atomic_read()/atomic_set(), normal read/write
operations are not atomic for KCSAN. KCSAN reports some value being changed
during a read/write.

In this report, KCSAN detected that __tty_hangup() changed the value of
filp->f_op from 0xffffffff84e91ed0 to 0xffffffff84e91dc0 at

  filp->f_op = &hung_up_tty_fops;

line when __fput() was reading the value of filp->f_op at

  if (file->f_op->release)

line.

Even if we implement the needed callbacks in hung_up_tty_fops,
KCSAN will continue reporting that the value of filp->f_op changes.

> 
> And why would data_race() help here?

data_race() tells KCSAN not to report.
data_race() is used when the race KCSAN checks is harmless.



>> @@ -182,7 +182,7 @@ int tty_alloc_file(struct file *file)
>>  {
>>  	struct tty_file_private *priv;
>>  
>> -	priv = kmalloc(sizeof(*priv), GFP_KERNEL);
>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> 
> Why is this zeroing out everything now?  Just because you added one
> bool?  Why not just set the bool properly instead?

Because I consider that this function is not performance critical where
avoid increasing code size by zeroing out everything is acceptable.



>> -static long hung_up_tty_compat_ioctl(struct file *file,
>> +static inline long hung_up_tty_compat_ioctl(struct file *file,
>>  				     unsigned int cmd, unsigned long arg)
>>  {
>>  	return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
>>  }
> 
> Marking these as inline, and then treating them as a function pointer,
> seems like a horrid way to work around a compiler warning.  As they
> really are not inline functions anymore, but yet the compiler doesn't
> know that.  Odds are once the compiler gets smarter, the warnings will
> return, so please, solve this properly.

Since this patch removes "struct file_operations hung_up_tty_fops"
which was the only source of treating as a function pointer,
these inlined functions are no longer treated as a function pointer.



>> @@ -619,7 +608,8 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
>>  			continue;
>>  		closecount++;
>>  		__tty_fasync(-1, filp, 0);	/* can't block */
>> -		filp->f_op = &hung_up_tty_fops;
>> +		/* Accept race with tty_hung_up_p() test. */
>> +		data_race(priv->hung = true);
> 
> Why accept it?  Say why it's not really an issue here.

Because whether tty_hung_up_p() sees true or false due to concurrent
access does not matter. The race KCSAN reported is harmless (unless
callbacks suddenly disappear).



>> @@ -743,7 +733,9 @@ void tty_vhangup_session(struct tty_struct *tty)
>>   */
>>  int tty_hung_up_p(struct file *filp)
>>  {
>> -	return (filp && filp->f_op == &hung_up_tty_fops);
>> +	return filp && filp->f_op == &tty_fops &&
>> +		/* Accept race with __tty_hangup(). */
>> +		data_race(((struct tty_file_private *) filp->private_data)->hung);
> 
> Same here.

Because whether __tty_hangup() already changed from false to true due to
concurrent access does not matter. The race KCSAN reported is harmless (unless
callbacks suddenly disappear).



>> @@ -911,6 +903,8 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
>>  	struct tty_struct *tty = file_tty(file);
>>  	struct tty_ldisc *ld;
>>  
>> +	if (tty_hung_up_p(file))
>> +		return hung_up_tty_read(iocb, to);
> 
> What happens if you hang up _right_ after this check?  There's no
> locking here, right?  Same everywhere else you have this pattern, you
> made the race window smaller, but it's still there from what I can see.

We cannot close the race window without introducing locking,
but we don't need to close the race window.

The race KCSAN found in this report is harmless, as long as callbacks
reachable via filp->f_op does not disappear.

This patch prevents filp->f_op from suddenly disappearing callbacks,
by not changing the value of filp->f_op.



>> @@ -255,6 +255,7 @@ struct tty_file_private {
>>  	struct tty_struct *tty;
>>  	struct file *file;
>>  	struct list_head list;
>> +	bool hung;
> 
> No hint as to what "hung" means here?

Whether __tty_hangup() was called or not.