[PATCH] n_tty: fix redirected_tty_write checks after write_iter conversion

[PATCH] n_tty: fix redirected_tty_write checks after write_iter conversion

[Sami Tolvanen]

Commit 9bb48c82aced ("tty: implement write_iter") converted the tty
layer to use write_iter. Fix the redirected_tty_write declaration
also in n_tty and change the comparisons to use write_iter instead of
write.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 drivers/tty/n_tty.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 319d68c8a5df..ee70937ac2b8 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2081,8 +2081,7 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	return 0;
 }
 
-extern ssize_t redirected_tty_write(struct file *, const char __user *,
-							size_t, loff_t *);
+extern ssize_t redirected_tty_write(struct kiocb *, struct iov_iter *);
 
 /**
  *	job_control		-	check job control
@@ -2105,7 +2104,7 @@ static int job_control(struct tty_struct *tty, struct file *file)
 	/* NOTE: not yet done after every sleep pending a thorough
 	   check of the logic of this change. -- jlc */
 	/* don't stop on /dev/console */
-	if (file->f_op->write == redirected_tty_write)
+	if (file->f_op->write_iter == redirected_tty_write)
 		return 0;
 
 	return __tty_check_change(tty, SIGTTIN);
@@ -2309,7 +2308,7 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
 	ssize_t retval = 0;
 
 	/* Job control check -- must be done at start (POSIX.1 7.1.1.4). */
-	if (L_TOSTOP(tty) && file->f_op->write != redirected_tty_write) {
+	if (L_TOSTOP(tty) && file->f_op->write_iter != redirected_tty_write) {
 		retval = tty_check_change(tty);
 		if (retval)
 			return retval;

base-commit: 6ee1d745b7c9fd573fba142a2efdad76a9f1cb04
-- 
2.30.0.280.ga3ce27912f-goog

Re: [PATCH] n_tty: fix redirected_tty_write checks after write_iter conversion

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On Mon, Jan 25, 2021 at 11:09 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> Commit 9bb48c82aced ("tty: implement write_iter") converted the tty
> layer to use write_iter. Fix the redirected_tty_write declaration
> also in n_tty and change the comparisons to use write_iter instead of
> write.

Duh.

Obvious ACK from me.

The only thing I'd ask is that the declaration for
redirected_tty_write() be moved to a proper header file
(<linux/tty.h>.

Because the reason I didn't notice this was literally that n_tty.c did
its own private 'extern' declaration of that function. Which is
horribly wrong, exactly because it doesn't then ever notice when the
declaration is changed.

So I'd suggest doing the patch as attached - but please keep Sami's
credit, this is purely a "declare the function in the proper place"
fix.

            Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 2590 bytes --]

 drivers/tty/n_tty.c  | 7 ++-----
 drivers/tty/tty_io.c | 2 --
 include/linux/tty.h  | 1 +
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 319d68c8a5df..219e85756171 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2081,9 +2081,6 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	return 0;
 }
 
-extern ssize_t redirected_tty_write(struct file *, const char __user *,
-							size_t, loff_t *);
-
 /**
  *	job_control		-	check job control
  *	@tty: tty
@@ -2105,7 +2102,7 @@ static int job_control(struct tty_struct *tty, struct file *file)
 	/* NOTE: not yet done after every sleep pending a thorough
 	   check of the logic of this change. -- jlc */
 	/* don't stop on /dev/console */
-	if (file->f_op->write == redirected_tty_write)
+	if (file->f_op->write_iter == redirected_tty_write)
 		return 0;
 
 	return __tty_check_change(tty, SIGTTIN);
@@ -2309,7 +2306,7 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
 	ssize_t retval = 0;
 
 	/* Job control check -- must be done at start (POSIX.1 7.1.1.4). */
-	if (L_TOSTOP(tty) && file->f_op->write != redirected_tty_write) {
+	if (L_TOSTOP(tty) && file->f_op->write_iter != redirected_tty_write) {
 		retval = tty_check_change(tty);
 		if (retval)
 			return retval;
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 4a208a95e921..48de20916ca7 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -144,10 +144,8 @@ DEFINE_MUTEX(tty_mutex);
 
 static ssize_t tty_read(struct file *, char __user *, size_t, loff_t *);
 static ssize_t tty_write(struct kiocb *, struct iov_iter *);
-ssize_t redirected_tty_write(struct kiocb *, struct iov_iter *);
 static __poll_t tty_poll(struct file *, poll_table *);
 static int tty_open(struct inode *, struct file *);
-long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 #ifdef CONFIG_COMPAT
 static long tty_compat_ioctl(struct file *file, unsigned int cmd,
 				unsigned long arg);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index c873f475f0a7..37803f3e6d49 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -421,6 +421,7 @@ extern void tty_kclose(struct tty_struct *tty);
 extern int tty_dev_name_to_number(const char *name, dev_t *number);
 extern int tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout);
 extern void tty_ldisc_unlock(struct tty_struct *tty);
+extern ssize_t redirected_tty_write(struct kiocb *, struct iov_iter *);
 #else
 static inline void tty_kref_put(struct tty_struct *tty)
 { }

Re: [PATCH] n_tty: fix redirected_tty_write checks after write_iter conversion

[Linus Torvalds]

On Mon, Jan 25, 2021 at 11:27 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Obvious ACK from me.

Greg - if you have nothing else lined up in the tty tree, I can take
this fix directly if it's easier.

And Sami - how did you actually notice? Some lint-like tool, or is
there something that actually broke from n_tty not handling a
redirected tty right?

            Linus

Re: [PATCH] n_tty: fix redirected_tty_write checks after write_iter conversion

[Greg Kroah-Hartman]

On Mon, Jan 25, 2021 at 11:45:12AM -0800, Linus Torvalds wrote:
> On Mon, Jan 25, 2021 at 11:27 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Obvious ACK from me.
> 
> Greg - if you have nothing else lined up in the tty tree, I can take
> this fix directly if it's easier.

I have nothing else that I know of, so yes, it is easier for you to take
it directly, thanks!

greg k-h

Re: [PATCH] n_tty: fix redirected_tty_write checks after write_iter conversion

[Linus Torvalds]

On Mon, Jan 25, 2021 at 11:52 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> I have nothing else that I know of, so yes, it is easier for you to take
> it directly, thanks!

It's top-of-tree right now, commit  9f12e37cae44.

And I just noticed I screwed up the formatting when editing the patch
- after pushing out.  I deleted the actual top line.

Darn.

            Linus

Re: [PATCH] n_tty: fix redirected_tty_write checks after write_iter conversion

[Sami Tolvanen]

On Mon, Jan 25, 2021 at 11:45 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Jan 25, 2021 at 11:27 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Obvious ACK from me.
>
> Greg - if you have nothing else lined up in the tty tree, I can take
> this fix directly if it's easier.
>
> And Sami - how did you actually notice? Some lint-like tool, or is
> there something that actually broke from n_tty not handling a
> redirected tty right?

Neither, I noticed this because the conflicting function declarations
broke Clang's Control Flow Integrity checking.

Sami

Re: [PATCH] n_tty: fix redirected_tty_write checks after write_iter conversion

[Linus Torvalds]

On Mon, Jan 25, 2021 at 12:03 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> Neither, I noticed this because the conflicting function declarations
> broke Clang's Control Flow Integrity checking.

Ahh, interesting. Is that automated somewhere, or are you running your
own special checks? It sounds like a useful thing.

I was thinking that maybe I should make some sparse-based cross-file
checker, but it sounds like -fsanitize=cfi (or whatever it is you do)
catches it.

              Linus

Re: [PATCH] n_tty: fix redirected_tty_write checks after write_iter conversion

[Sami Tolvanen]

On Mon, Jan 25, 2021 at 12:08 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Jan 25, 2021 at 12:03 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > Neither, I noticed this because the conflicting function declarations
> > broke Clang's Control Flow Integrity checking.
>
> Ahh, interesting. Is that automated somewhere, or are you running your
> own special checks? It sounds like a useful thing.

I’m running a continuous integration script locally, which tests a few
basic kernel configurations with CFI to ensure they compile and boot.
We’re using CFI in Android kernels, so this helps catch issues before
they reach stable kernels.

> I was thinking that maybe I should make some sparse-based cross-file
> checker, but it sounds like -fsanitize=cfi (or whatever it is you do)
> catches it.

That might still be useful, because CFI only adds runtime checking.
It’s primarily a mitigation against code reuse attacks, but it does
find these types of issues occasionally.

Sami