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.
v2 - addressed some review comments on the 2/2 patch.
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
From: Linus Torvalds <[email protected]>
After we are done with the tty buffer, zero it out.
Reported-by: aszlig <[email protected]>
Tested-by: Milan Broz <[email protected]>
Tested-by: aszlig <[email protected]>
Cc: Willy Tarreau <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
v2 - no change
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
From: Greg KH <[email protected]>
If we are not echoing the data to userspace or the console is in icanon
mode, 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 <[email protected]>
Tested-by: Milan Broz <[email protected]>
Tested-by: aszlig <[email protected]>
Cc: Willy Tarreau <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
v2 - update changelog comment to mention icanon
address the fact that copy_to_user() might not copy the whole buffer
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..3ad460219fd6 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 - uncopied);
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 - uncopied);
+ 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
On 04/10/2018 20:06, Greg Kroah-Hartman wrote:
> 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.
>
> v2 - addressed some review comments on the 2/2 patch.
Hello Greg,
together with our intern we re-tested both patches and it fixes the reported problem,
so, if it helps anything, you can add
Tested-by: Milan Broz <[email protected]>
Tested-by: Daniel Zatovic <[email protected]>
Do you plan to add this to linux-next?
Thanks!
Milan
>
> 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(-)
>
On Wed, Oct 10, 2018 at 08:20:06PM +0200, Milan Broz wrote:
> On 04/10/2018 20:06, Greg Kroah-Hartman wrote:
> > 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.
> >
> > v2 - addressed some review comments on the 2/2 patch.
>
> Hello Greg,
>
> together with our intern we re-tested both patches and it fixes the reported problem,
> so, if it helps anything, you can add
>
> Tested-by: Milan Broz <[email protected]>
> Tested-by: Daniel Zatovic <[email protected]>
>
> Do you plan to add this to linux-next?
Yes, I'll queue it up soon, thanks for testing!
greg k-h