All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] tty: erase buffers when the kernel is done with it.
@ 2018-10-02 17:17 Greg Kroah-Hartman
  2018-10-02 17:17 ` [PATCH 1/2] tty: wipe buffer Greg Kroah-Hartman
  2018-10-02 17:17 ` [PATCH 2/2] tty: wipe buffer if not echoing data Greg Kroah-Hartman
  0 siblings, 2 replies; 4+ messages in thread
From: Greg Kroah-Hartman @ 2018-10-02 17:17 UTC (permalink / raw)
  To: linux-serial
  Cc: linux-kernel, jslaby, aszlig, gmazyland, torvalds, w, Greg Kroah-Hartman

azlig and Milan Broz reported that when the tty layer is done with a
buffer, the data can hang around in it for a very long time.  That
sometimes can "leak" to userspace under some conditions.

Because of this, just zero out the data after the tty layer is finished
with it, for buffers that we "think" should be zeroed out.

Greg Kroah-Hartman (1):
  tty: wipe buffer if not echoing data

Linus Torvalds (1):
  tty: wipe buffer.

 drivers/tty/n_tty.c      | 20 +++++++++++++++++---
 drivers/tty/tty_buffer.c |  6 +++++-
 2 files changed, 22 insertions(+), 4 deletions(-)

-- 
2.19.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] tty: wipe buffer.
  2018-10-02 17:17 [PATCH 0/2] tty: erase buffers when the kernel is done with it Greg Kroah-Hartman
@ 2018-10-02 17:17 ` Greg Kroah-Hartman
  2018-10-02 17:17 ` [PATCH 2/2] tty: wipe buffer if not echoing data Greg Kroah-Hartman
  1 sibling, 0 replies; 4+ messages in thread
From: Greg Kroah-Hartman @ 2018-10-02 17:17 UTC (permalink / raw)
  To: linux-serial
  Cc: linux-kernel, jslaby, aszlig, gmazyland, torvalds, w, Greg Kroah-Hartman

From: Linus Torvalds <torvalds@linux-foundation.org>

After we are done with the tty buffer, zero it out.

Reported-by: aszlig <aszlig@nix.build>
Tested-by: Milan Broz <gmazyland@gmail.com>
Tested-by: aszlig <aszlig@nix.build>
Cc: Willy Tarreau <w@1wt.eu>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/tty_buffer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index c996b6859c5e..ae3ce330200e 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -468,11 +468,15 @@ receive_buf(struct tty_port *port, struct tty_buffer *head, int count)
 {
 	unsigned char *p = char_buf_ptr(head, head->read);
 	char	      *f = NULL;
+	int n;
 
 	if (~head->flags & TTYB_NORMAL)
 		f = flag_buf_ptr(head, head->read);
 
-	return port->client_ops->receive_buf(port, p, f, count);
+	n = port->client_ops->receive_buf(port, p, f, count);
+	if (n > 0)
+		memset(p, 0, n);
+	return n;
 }
 
 /**
-- 
2.19.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] tty: wipe buffer if not echoing data
  2018-10-02 17:17 [PATCH 0/2] tty: erase buffers when the kernel is done with it Greg Kroah-Hartman
  2018-10-02 17:17 ` [PATCH 1/2] tty: wipe buffer Greg Kroah-Hartman
@ 2018-10-02 17:17 ` Greg Kroah-Hartman
  2018-10-03  6:55   ` Milan Broz
  1 sibling, 1 reply; 4+ messages in thread
From: Greg Kroah-Hartman @ 2018-10-02 17:17 UTC (permalink / raw)
  To: linux-serial
  Cc: linux-kernel, jslaby, aszlig, gmazyland, torvalds, w, Greg KH,
	Greg Kroah-Hartman

From: Greg KH <greg@kroah.com>

If we are not echoing the data to userspace, then perhaps it is a
"secret" so we should wipe it once we are done with it.

This mirrors the logic that the audit code has.

Reported-by: aszlig <aszlig@nix.build>
Tested-by: Milan Broz <gmazyland@gmail.com>
Tested-by: aszlig <aszlig@nix.build>
Cc: Willy Tarreau <w@1wt.eu>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/n_tty.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 431742201709..d259a6585dbe 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -152,17 +152,28 @@ static inline unsigned char *echo_buf_addr(struct n_tty_data *ldata, size_t i)
 	return &ldata->echo_buf[i & (N_TTY_BUF_SIZE - 1)];
 }
 
+/* If we are not echoing the data, perhaps this is a secret so erase it */
+static void zero_buffer(struct tty_struct *tty, u8 *buffer, int size)
+{
+	bool icanon = !!L_ICANON(tty);
+	bool no_echo = !L_ECHO(tty);
+
+	if (icanon && no_echo)
+		memset(buffer, 0x00, size);
+}
+
 static int tty_copy_to_user(struct tty_struct *tty, void __user *to,
 			    size_t tail, size_t n)
 {
 	struct n_tty_data *ldata = tty->disc_data;
 	size_t size = N_TTY_BUF_SIZE - tail;
-	const void *from = read_buf_addr(ldata, tail);
+	void *from = read_buf_addr(ldata, tail);
 	int uncopied;
 
 	if (n > size) {
 		tty_audit_add_data(tty, from, size);
 		uncopied = copy_to_user(to, from, size);
+		zero_buffer(tty, from, size);
 		if (uncopied)
 			return uncopied;
 		to += size;
@@ -171,7 +182,9 @@ static int tty_copy_to_user(struct tty_struct *tty, void __user *to,
 	}
 
 	tty_audit_add_data(tty, from, n);
-	return copy_to_user(to, from, n);
+	uncopied = copy_to_user(to, from, n);
+	zero_buffer(tty, from, n);
+	return uncopied;
 }
 
 /**
@@ -1960,11 +1973,12 @@ static int copy_from_read_buf(struct tty_struct *tty,
 	n = min(head - ldata->read_tail, N_TTY_BUF_SIZE - tail);
 	n = min(*nr, n);
 	if (n) {
-		const unsigned char *from = read_buf_addr(ldata, tail);
+		unsigned char *from = read_buf_addr(ldata, tail);
 		retval = copy_to_user(*b, from, n);
 		n -= retval;
 		is_eof = n == 1 && *from == EOF_CHAR(tty);
 		tty_audit_add_data(tty, from, n);
+		zero_buffer(tty, from, n);
 		smp_store_release(&ldata->read_tail, ldata->read_tail + n);
 		/* Turn single EOF into zero-length read */
 		if (L_EXTPROC(tty) && ldata->icanon && is_eof &&
-- 
2.19.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] tty: wipe buffer if not echoing data
  2018-10-02 17:17 ` [PATCH 2/2] tty: wipe buffer if not echoing data Greg Kroah-Hartman
@ 2018-10-03  6:55   ` Milan Broz
  0 siblings, 0 replies; 4+ messages in thread
From: Milan Broz @ 2018-10-03  6:55 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-serial
  Cc: linux-kernel, jslaby, aszlig, torvalds, w, Greg KH

On 02/10/2018 19:17, Greg Kroah-Hartman wrote:
> From: Greg KH <greg@kroah.com>
> 
> If we are not echoing the data to userspace, then perhaps it is a
> "secret" so we should wipe it once we are done with it.

Just to explain our test case for cryptsetup, where aszlig initially reported it:

cryptsetup reads a passphrase from terminal, derives a candidate key for keyslot
decryption and immediately wipes the passphrase from memory, because it is
not needed anymore.

And here is the problem - the kernel keeps this passphrase in a tty buffer memory
that is no longer available to userspace. It can be retrieved from memory dump
later, even after the crypt mapping is destroyed.
Almost all userspace tools working with passphrases through tty access have
the same problem here.

> This mirrors the logic that the audit code has.
> 
> Reported-by: aszlig <aszlig@nix.build>
> Tested-by: Milan Broz <gmazyland@gmail.com>
> Tested-by: aszlig <aszlig@nix.build>
> Cc: Willy Tarreau <w@1wt.eu>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/tty/n_tty.c | 20 +++++++++++++++++---
...

>  static int tty_copy_to_user(struct tty_struct *tty, void __user *to,
>  			    size_t tail, size_t n)
>  {
>  	struct n_tty_data *ldata = tty->disc_data;
>  	size_t size = N_TTY_BUF_SIZE - tail;
> -	const void *from = read_buf_addr(ldata, tail);
> +	void *from = read_buf_addr(ldata, tail);
>  	int uncopied;
>  
>  	if (n > size) {
>  		tty_audit_add_data(tty, from, size);
>  		uncopied = copy_to_user(to, from, size);
> +		zero_buffer(tty, from, size);

I think Linus mentioned in some previous mail that there should be
  zero_buffer(tty, from, size - uncopied);
to avoid wiping of yet uncopied data.

I tested the unmodified version though...

>  		if (uncopied)
>  			return uncopied;
>  		to += size;
> @@ -171,7 +182,9 @@ static int tty_copy_to_user(struct tty_struct *tty, void __user *to,
>  	}
>  
>  	tty_audit_add_data(tty, from, n);
> -	return copy_to_user(to, from, n);
> +	uncopied = copy_to_user(to, from, n);
> +	zero_buffer(tty, from, n);
> +	return uncopied;
>  }


Milan


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-10-03  6:55 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-02 17:17 [PATCH 0/2] tty: erase buffers when the kernel is done with it Greg Kroah-Hartman
2018-10-02 17:17 ` [PATCH 1/2] tty: wipe buffer Greg Kroah-Hartman
2018-10-02 17:17 ` [PATCH 2/2] tty: wipe buffer if not echoing data Greg Kroah-Hartman
2018-10-03  6:55   ` Milan Broz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.