Hi!
In a recent discussion, Linus and Al Viro said quite a bit of expletives
about __put_user() and __get_user(), that it's a bad interface that's
almost always the wrong thing to use:
https://marc.info/?l=linux-kernel&m=149463725626316&w=2
https://marc.info/?l=linux-kernel&m=149465866929092&w=2
Here's a few patches applying the lessons from that discussion to vt.
None of the uses is performance-critical, but at least we get a nice bit
of code simplification. And, it's a start of manual review + conversion
that Al Viro wants.
Adam Borowski (5):
vt: use copy_from/to_user instead of __get/put_user for scrnmap ioctls
vt: fix unchecked __put_user() in tioclinux ioctls
vt: use copy_to_user instead of __put_user in GIO_UNIMAP ioctl
vt: use memdup_user in PIO_UNIMAP ioctl
vt: drop access_ok() calls in unimap ioctls
drivers/tty/vt/consolemap.c | 56 ++++++++++++++++----------------------------------------
drivers/tty/vt/vt.c | 6 +++---
drivers/tty/vt/vt_ioctl.c | 8 --------
3 files changed, 19 insertions(+), 51 deletions(-)
--
⢀⣴⠾⠻⢶⣦⠀ A tit a day keeps the vet away.
⣾⠁⢰⠒⠀⣿⡁
⢿⡄⠘⠷⠚⠋⠀ (Rejoice as my small-animal-murder-machine got unbroken after
⠈⠳⣄⠀⠀⠀⠀ nearly two years of no catch!)
Linus wants to get rid of these functions, and these uses are especially
egregious: they copy a big linear array element by element.
Signed-off-by: Adam Borowski <[email protected]>
---
drivers/tty/vt/consolemap.c | 31 +++++++------------------------
1 file changed, 7 insertions(+), 24 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 1f6e17fc3fb0..1361f2a8b832 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -322,15 +322,13 @@ int con_set_trans_old(unsigned char __user * arg)
{
int i;
unsigned short inbuf[E_TABSZ];
+ unsigned char ubuf[E_TABSZ];
- if (!access_ok(VERIFY_READ, arg, E_TABSZ))
+ if (copy_from_user(ubuf, arg, E_TABSZ))
return -EFAULT;
- for (i = 0; i < E_TABSZ ; i++) {
- unsigned char uc;
- __get_user(uc, arg+i);
- inbuf[i] = UNI_DIRECT_BASE | uc;
- }
+ for (i = 0; i < E_TABSZ ; i++)
+ inbuf[i] = UNI_DIRECT_BASE | ubuf[i];
console_lock();
memcpy(translations[USER_MAP], inbuf, sizeof(inbuf));
@@ -345,9 +343,6 @@ int con_get_trans_old(unsigned char __user * arg)
unsigned short *p = translations[USER_MAP];
unsigned char outbuf[E_TABSZ];
- if (!access_ok(VERIFY_WRITE, arg, E_TABSZ))
- return -EFAULT;
-
console_lock();
for (i = 0; i < E_TABSZ ; i++)
{
@@ -356,22 +351,16 @@ int con_get_trans_old(unsigned char __user * arg)
}
console_unlock();
- for (i = 0; i < E_TABSZ ; i++)
- __put_user(outbuf[i], arg+i);
- return 0;
+ return copy_to_user(arg, outbuf, sizeof(outbuf)) ? -EFAULT : 0;
}
int con_set_trans_new(ushort __user * arg)
{
- int i;
unsigned short inbuf[E_TABSZ];
- if (!access_ok(VERIFY_READ, arg, E_TABSZ*sizeof(unsigned short)))
+ if (copy_from_user(inbuf, arg, sizeof(inbuf)))
return -EFAULT;
- for (i = 0; i < E_TABSZ ; i++)
- __get_user(inbuf[i], arg+i);
-
console_lock();
memcpy(translations[USER_MAP], inbuf, sizeof(inbuf));
update_user_maps();
@@ -381,19 +370,13 @@ int con_set_trans_new(ushort __user * arg)
int con_get_trans_new(ushort __user * arg)
{
- int i;
unsigned short outbuf[E_TABSZ];
- if (!access_ok(VERIFY_WRITE, arg, E_TABSZ*sizeof(unsigned short)))
- return -EFAULT;
-
console_lock();
memcpy(outbuf, translations[USER_MAP], sizeof(outbuf));
console_unlock();
- for (i = 0; i < E_TABSZ ; i++)
- __put_user(outbuf[i], arg+i);
- return 0;
+ return copy_to_user(arg, outbuf, sizeof(outbuf)) ? -EFAULT : 0;
}
/*
--
2.11.0
Only read access is checked before this call.
Actually, at the moment this is not an issue, as every in-tree arch does
the same manual checks for VERIFY_READ vs VERIFY_WRITE, relying on the MMU
to tell them apart, but this wasn't the case in the past and may happen
again on some odd arch in the future.
If anyone cares about 3.7 and earlier, this is a security hole (untested)
on real 80386 CPUs.
Signed-off-by: Adam Borowski <[email protected]>
CC: [email protected] # v3.7-
---
drivers/tty/vt/vt.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9309c7da660a..2ebaba16f785 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2709,13 +2709,13 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
* related to the kernel should not use this.
*/
data = vt_get_shift_state();
- ret = __put_user(data, p);
+ ret = put_user(data, p);
break;
case TIOCL_GETMOUSEREPORTING:
console_lock(); /* May be overkill */
data = mouse_reporting();
console_unlock();
- ret = __put_user(data, p);
+ ret = put_user(data, p);
break;
case TIOCL_SETVESABLANK:
console_lock();
@@ -2724,7 +2724,7 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
break;
case TIOCL_GETKMSGREDIRECT:
data = vt_get_kmsg_redirect();
- ret = __put_user(data, p);
+ ret = put_user(data, p);
break;
case TIOCL_SETKMSGREDIRECT:
if (!capable(CAP_SYS_ADMIN)) {
--
2.11.0
Done by copy_{from,to}_user().
Signed-off-by: Adam Borowski <[email protected]>
---
drivers/tty/vt/vt_ioctl.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 0cbfe1ff6f6c..96d389cb506c 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -266,10 +266,6 @@ do_unimap_ioctl(int cmd, struct unimapdesc __user *user_ud, int perm, struct vc_
if (copy_from_user(&tmp, user_ud, sizeof tmp))
return -EFAULT;
- if (tmp.entries)
- if (!access_ok(VERIFY_WRITE, tmp.entries,
- tmp.entry_ct*sizeof(struct unipair)))
- return -EFAULT;
switch (cmd) {
case PIO_UNIMAP:
if (!perm)
@@ -1170,10 +1166,6 @@ compat_unimap_ioctl(unsigned int cmd, struct compat_unimapdesc __user *user_ud,
if (copy_from_user(&tmp, user_ud, sizeof tmp))
return -EFAULT;
tmp_entries = compat_ptr(tmp.entries);
- if (tmp_entries)
- if (!access_ok(VERIFY_WRITE, tmp_entries,
- tmp.entry_ct*sizeof(struct unipair)))
- return -EFAULT;
switch (cmd) {
case PIO_UNIMAP:
if (!perm)
--
2.11.0
Again, a nice linear transfer that simplifies the code.
Signed-off-by: Adam Borowski <[email protected]>
---
drivers/tty/vt/consolemap.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index c6a692f63a9b..a5f88cf0f61d 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -540,14 +540,9 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
if (!ct)
return 0;
- unilist = kmalloc_array(ct, sizeof(struct unipair), GFP_KERNEL);
- if (!unilist)
- return -ENOMEM;
-
- for (i = ct, plist = unilist; i; i--, plist++, list++) {
- __get_user(plist->unicode, &list->unicode);
- __get_user(plist->fontpos, &list->fontpos);
- }
+ unilist = memdup_user(list, ct * sizeof(struct unipair));
+ if (IS_ERR(unilist))
+ return PTR_ERR(unilist);
console_lock();
--
2.11.0
A nice big linear transfer, no need to flip stac/PAN/etc every half-entry.
Also, yay __put_user() after checking only read.
Signed-off-by: Adam Borowski <[email protected]>
---
drivers/tty/vt/consolemap.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 1361f2a8b832..c6a692f63a9b 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -740,11 +740,11 @@ EXPORT_SYMBOL(con_copy_unimap);
*/
int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct unipair __user *list)
{
- int i, j, k;
+ int i, j, k, ret = 0;
ushort ect;
u16 **p1, *p2;
struct uni_pagedir *p;
- struct unipair *unilist, *plist;
+ struct unipair *unilist;
unilist = kmalloc_array(ct, sizeof(struct unipair), GFP_KERNEL);
if (!unilist)
@@ -775,13 +775,11 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
}
}
console_unlock();
- for (i = min(ect, ct), plist = unilist; i; i--, list++, plist++) {
- __put_user(plist->unicode, &list->unicode);
- __put_user(plist->fontpos, &list->fontpos);
- }
- __put_user(ect, uct);
+ if (copy_to_user(list, unilist, min(ect, ct) * sizeof(struct unipair)))
+ ret = -EFAULT;
+ put_user(ect, uct);
kfree(unilist);
- return ((ect <= ct) ? 0 : -ENOMEM);
+ return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
}
/*
--
2.11.0
On Sat, Jun 03, 2017 at 09:32:55AM +0200, Adam Borowski wrote:
> Hi!
> In a recent discussion, Linus and Al Viro said quite a bit of expletives
> about __put_user() and __get_user(), that it's a bad interface that's
> almost always the wrong thing to use:
> https://marc.info/?l=linux-kernel&m=149463725626316&w=2
> https://marc.info/?l=linux-kernel&m=149465866929092&w=2
>
> Here's a few patches applying the lessons from that discussion to vt.
> None of the uses is performance-critical, but at least we get a nice bit
> of code simplification. And, it's a start of manual review + conversion
> that Al Viro wants.
Ah, nice work, at first glance these all look good to me. I'll queue
them up on Monday.
greg k-h
On Sun, Jun 04, 2017 at 12:42:52AM +0900, Greg Kroah-Hartman wrote:
> On Sat, Jun 03, 2017 at 09:32:55AM +0200, Adam Borowski wrote:
> > Hi!
> > In a recent discussion, Linus and Al Viro said quite a bit of expletives
> > about __put_user() and __get_user(), that it's a bad interface that's
> > almost always the wrong thing to use:
> > https://marc.info/?l=linux-kernel&m=149463725626316&w=2
> > https://marc.info/?l=linux-kernel&m=149465866929092&w=2
> >
> > Here's a few patches applying the lessons from that discussion to vt.
> > None of the uses is performance-critical, but at least we get a nice bit
> > of code simplification. And, it's a start of manual review + conversion
> > that Al Viro wants.
>
> Ah, nice work, at first glance these all look good to me. I'll queue
> them up on Monday.
Could you put that into a separate no-rebase branch? Or I could do that
in vfs.git, for that matter...
On Mon, Jun 05, 2017 at 07:13:50AM +0100, Al Viro wrote:
> On Sun, Jun 04, 2017 at 12:42:52AM +0900, Greg Kroah-Hartman wrote:
> > On Sat, Jun 03, 2017 at 09:32:55AM +0200, Adam Borowski wrote:
> > > Hi!
> > > In a recent discussion, Linus and Al Viro said quite a bit of expletives
> > > about __put_user() and __get_user(), that it's a bad interface that's
> > > almost always the wrong thing to use:
> > > https://marc.info/?l=linux-kernel&m=149463725626316&w=2
> > > https://marc.info/?l=linux-kernel&m=149465866929092&w=2
> > >
> > > Here's a few patches applying the lessons from that discussion to vt.
> > > None of the uses is performance-critical, but at least we get a nice bit
> > > of code simplification. And, it's a start of manual review + conversion
> > > that Al Viro wants.
> >
> > Ah, nice work, at first glance these all look good to me. I'll queue
> > them up on Monday.
>
> Could you put that into a separate no-rebase branch? Or I could do that
> in vfs.git, for that matter...
If you just want to take these in vfs.git feel free to, I don't mind at
all:
Acked-by: Greg Kroah-Hartman <[email protected]>
that's probably the easiest.
thanks,
greg k-h
> @@ -775,13 +775,11 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
> }
> }
> console_unlock();
> - for (i = min(ect, ct), plist = unilist; i; i--, list++, plist++) {
> - __put_user(plist->unicode, &list->unicode);
> - __put_user(plist->fontpos, &list->fontpos);
> - }
> - __put_user(ect, uct);
> + if (copy_to_user(list, unilist, min(ect, ct) * sizeof(struct unipair)))
> + ret = -EFAULT;
> + put_user(ect, uct);
> kfree(unilist);
> - return ((ect <= ct) ? 0 : -ENOMEM);
> + return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
> }
The rest looks good but that line needs taking out and shooting.
Alan
On Mon, Jun 05, 2017 at 05:34:16PM +0100, Alan Cox wrote:
> > @@ -775,13 +775,11 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
> > }
> > }
> > console_unlock();
> > - for (i = min(ect, ct), plist = unilist; i; i--, list++, plist++) {
> > - __put_user(plist->unicode, &list->unicode);
> > - __put_user(plist->fontpos, &list->fontpos);
> > - }
> > - __put_user(ect, uct);
> > + if (copy_to_user(list, unilist, min(ect, ct) * sizeof(struct unipair)))
> > + ret = -EFAULT;
> > + put_user(ect, uct);
> > kfree(unilist);
> > - return ((ect <= ct) ? 0 : -ENOMEM);
> > + return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
> > }
>
>
> The rest looks good but that line needs taking out and shooting.
The rest of that function is also Not Nice(tm). Creative indentation, unchecked
put_user() result... How about this on top of Adam's commit? One thing I'm
not sure about is this -ENOMEM return on overflow; there's no way for caller
to tell it from allocation failure, so what's the point copying the list out
in that case? I've kept the behaviour as-is, but...
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index c6a692f63a9b..73ef478c7e68 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -731,6 +731,36 @@ int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc)
}
EXPORT_SYMBOL(con_copy_unimap);
+static int fill_unilist(struct unipair *list, struct uni_pagedir *dir, u16 ct)
+{
+ int i, j, k, n;
+
+ if (!dir)
+ return 0;
+
+ for (n = i = 0; i < 32; i++) {
+ u16 **p1 = dir->uni_pgdir[i];
+ if (!p1)
+ continue;
+ for (j = 0; j < 32; j++) {
+ u16 *p2 = p1[j];
+ if (!p2)
+ continue;
+ for (k = 0; k < 64; k++) {
+ u16 pos = p2[k];
+ if (pos >= MAX_GLYPH)
+ continue;
+ if (unlikely(n == ct))
+ return -1;
+ list[n].unicode = (i<<11) + (j<<6) + k;
+ list[n].fontpos = pos;
+ n++;
+ }
+ }
+ }
+ return n;
+}
+
/**
* con_get_unimap - get the unicode map
* @vc: the console to read from
@@ -740,46 +770,29 @@ EXPORT_SYMBOL(con_copy_unimap);
*/
int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct unipair __user *list)
{
- int i, j, k, ret = 0;
- ushort ect;
- u16 **p1, *p2;
- struct uni_pagedir *p;
struct unipair *unilist;
+ ushort ect;
+ int n, ret = 0;
unilist = kmalloc_array(ct, sizeof(struct unipair), GFP_KERNEL);
if (!unilist)
return -ENOMEM;
console_lock();
-
- ect = 0;
- if (*vc->vc_uni_pagedir_loc) {
- p = *vc->vc_uni_pagedir_loc;
- for (i = 0; i < 32; i++) {
- p1 = p->uni_pgdir[i];
- if (p1)
- for (j = 0; j < 32; j++) {
- p2 = *(p1++);
- if (p2)
- for (k = 0; k < 64; k++, p2++) {
- if (*p2 >= MAX_GLYPH)
- continue;
- if (ect < ct) {
- unilist[ect].unicode =
- (i<<11)+(j<<6)+k;
- unilist[ect].fontpos = *p2;
- }
- ect++;
- }
- }
- }
- }
+ n = fill_unilist(unilist, *vc->vc_uni_pagedir_loc, ct);
console_unlock();
- if (copy_to_user(list, unilist, min(ect, ct) * sizeof(struct unipair)))
+ if (n < 0) {
+ ect = ct;
+ ret = -ENOMEM;
+ } else {
+ ect = n;
+ ret = 0;
+ }
+ if (copy_to_user(list, unilist, ect * sizeof(struct unipair)) ||
+ put_user(ect, uct))
ret = -EFAULT;
- put_user(ect, uct);
kfree(unilist);
- return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
+ return ret;
}
/*
On Mon, Jun 05, 2017 at 07:13:50AM +0100, Al Viro wrote:
> On Sun, Jun 04, 2017 at 12:42:52AM +0900, Greg Kroah-Hartman wrote:
> > On Sat, Jun 03, 2017 at 09:32:55AM +0200, Adam Borowski wrote:
> > > Hi!
> > > In a recent discussion, Linus and Al Viro said quite a bit of expletives
> > > about __put_user() and __get_user(), that it's a bad interface that's
> > > almost always the wrong thing to use:
> > > https://marc.info/?l=linux-kernel&m=149463725626316&w=2
> > > https://marc.info/?l=linux-kernel&m=149465866929092&w=2
> > >
> > > Here's a few patches applying the lessons from that discussion to vt.
> > > None of the uses is performance-critical, but at least we get a nice bit
> > > of code simplification. And, it's a start of manual review + conversion
> > > that Al Viro wants.
> >
> > Ah, nice work, at first glance these all look good to me. I'll queue
> > them up on Monday.
>
> Could you put that into a separate no-rebase branch? Or I could do that
> in vfs.git, for that matter...
Yes, here's a tag/branch for you to pull from that will not go away
until 4.13-rc1.
The following changes since commit 3c2993b8c6143d8a5793746a54eba8f86f95240f:
Linux 4.12-rc4 (2017-06-04 16:47:43 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/ tags/vt_copy_cleanup_tag
for you to fetch changes up to f8564c93e0907651e21d586920e629227bb0d024:
vt: drop access_ok() calls in unimap ioctls (2017-06-09 11:07:36 +0200)
----------------------------------------------------------------
vt: copy/from_to cleanup for vt code for Al to pull from.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
----------------------------------------------------------------
Adam Borowski (5):
vt: use copy_from/to_user instead of __get/put_user for scrnmap ioctls
vt: fix unchecked __put_user() in tioclinux ioctls
vt: use copy_to_user instead of __put_user in GIO_UNIMAP ioctl
vt: use memdup_user in PIO_UNIMAP ioctl
vt: drop access_ok() calls in unimap ioctls
drivers/tty/vt/consolemap.c | 56 +++++++++++++--------------------------------
drivers/tty/vt/vt.c | 6 ++---
drivers/tty/vt/vt_ioctl.c | 8 -------
3 files changed, 19 insertions(+), 51 deletions(-)