1) ECHO_IOC_GET_STRIPE starts with
copy_to_user (ulsm, lsm, sizeof(*ulsm)), where ulsm is a user-supplied
pointer to struct lov_stripe_md. Which starts with
struct lov_stripe_md {
atomic_t lsm_refc;
spinlock_t lsm_lock;
and since sizeof(spinlock_t) depends on a slew of CONFIG_..., so do the
offsets of everything after it. May I politely inquire how the hell
does it manage to be of any use for userland code?
2) echo_copyout_lsm() proceeds to do the following:
for (i = 0; i < lsm->lsm_stripe_count; i++) {
if (copy_to_user (ulsm->lsm_oinfo[i], lsm->lsm_oinfo[i],
sizeof(lsm->lsm_oinfo[0])))
return -EFAULT;
}
What do you think will happen if &(ulsm->lsm_oinfo) happens to be on a page
boundary, with the next page unmapped? Or, for that matter, what happens
if that gets used on an architecture with separate address spaces for kernel
and userland. Sparc, for example... That one is trivial to fix - it's
just missing get_user(up, ulsm->lsm_oinfo + i), with copy_to_user(up, ....)
following it.
3) echo_copyin_lsm() has the same issues (both of them).
4) fld_proc_hash_seq_write() does this:
if (!strncmp(fld_hash[i].fh_name, buffer, count)) {
'buffer' is a userland pointer - argument of write(2), actually.
5) ll_fiemap() does
memcpy(fieinfo->fi_extents_start, &fiemap->fm_extents[0],
fiemap->fm_mapped_extents *
sizeof(struct ll_fiemap_extent));
It's _not_ a nice thing to do, seeing that fi_extents_start is a userland
pointer. Granted, it has passed access_ok() in ioctl_fiemap(), so it's
not an instant roothole on x86. On anything with separate ASI for kernel
and userland it might very well be, depending on whether any kernel addresses
pass access_ok() there. parisc, for example, has access_ok() always 1 and
there it *is* a roothole. And it's certainly oopsable on x86.
Incidentally, WTF are ll_fiemap_extent and ll_user_fiemap? AFAICS these
are identical copies on include/uapi/linux/fiemap.h stuff, which had been
there for 6 years already...
Anyway, fixes for missing get_user() and for strncmp() on userland pointers
follow. The rest is a bit trickier.
Al, really annoyed by swimming through the lustre sewerful of ioctls...
>From fee276ea51f61386438e8e65f8e39babad8c6a25 Mon Sep 17 00:00:00 2001
From: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Date: Sun, 30 Nov 2014 00:12:37 -0500
Subject: [PATCH 1/2] lustre: strncmp() on user-supplied address is a Bad
Thing(tm)
Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
---
drivers/staging/lustre/lustre/fld/lproc_fld.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/lustre/lustre/fld/lproc_fld.c b/drivers/staging/lustre/lustre/fld/lproc_fld.c
index 95e7de1..3b6e13f 100644
--- a/drivers/staging/lustre/lustre/fld/lproc_fld.c
+++ b/drivers/staging/lustre/lustre/fld/lproc_fld.c
@@ -92,16 +92,21 @@ fld_proc_hash_seq_write(struct file *file, const char *buffer,
{
struct lu_client_fld *fld;
struct lu_fld_hash *hash = NULL;
+ char *s;
int i;
fld = ((struct seq_file *)file->private_data)->private;
LASSERT(fld != NULL);
+ s = memdup_user(buffer, count);
+ if (IS_ERR(s))
+ return PTR_ERR(s);
+
for (i = 0; fld_hash[i].fh_name != NULL; i++) {
if (count != strlen(fld_hash[i].fh_name))
continue;
- if (!strncmp(fld_hash[i].fh_name, buffer, count)) {
+ if (!strncmp(fld_hash[i].fh_name, s, count)) {
hash = &fld_hash[i];
break;
}
@@ -115,6 +120,7 @@ fld_proc_hash_seq_write(struct file *file, const char *buffer,
CDEBUG(D_INFO, "%s: Changed hash to \"%s\"\n",
fld->lcf_name, hash->fh_name);
}
+ kfree(s);
return count;
}
--
1.7.10.4
>From fc00a7396d279f77ef192fb442dc05daecb6136d Mon Sep 17 00:00:00 2001
From: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Date: Sun, 30 Nov 2014 16:02:33 -0500
Subject: [PATCH 2/2] lustre: echo_copy.._lsm() dereferences userland pointers
directly
missing get_user()
Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
---
.../staging/lustre/lustre/obdecho/echo_client.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/lustre/lustre/obdecho/echo_client.c b/drivers/staging/lustre/lustre/obdecho/echo_client.c
index 98e4290..373b2a3 100644
--- a/drivers/staging/lustre/lustre/obdecho/echo_client.c
+++ b/drivers/staging/lustre/lustre/obdecho/echo_client.c
@@ -1251,6 +1251,7 @@ static int
echo_copyout_lsm (struct lov_stripe_md *lsm, void *_ulsm, int ulsm_nob)
{
struct lov_stripe_md *ulsm = _ulsm;
+ struct lov_oinfo **p;
int nob, i;
nob = offsetof (struct lov_stripe_md, lsm_oinfo[lsm->lsm_stripe_count]);
@@ -1260,9 +1261,10 @@ echo_copyout_lsm (struct lov_stripe_md *lsm, void *_ulsm, int ulsm_nob)
if (copy_to_user (ulsm, lsm, sizeof(*ulsm)))
return -EFAULT;
- for (i = 0; i < lsm->lsm_stripe_count; i++) {
- if (copy_to_user (ulsm->lsm_oinfo[i], lsm->lsm_oinfo[i],
- sizeof(lsm->lsm_oinfo[0])))
+ for (i = 0, p = lsm->lsm_oinfo; i < lsm->lsm_stripe_count; i++, p++) {
+ struct lov_oinfo __user *up;
+ if (get_user(up, ulsm->lsm_oinfo + i) ||
+ copy_to_user(up, *p, sizeof(struct lov_oinfo)))
return -EFAULT;
}
return 0;
@@ -1270,9 +1272,10 @@ echo_copyout_lsm (struct lov_stripe_md *lsm, void *_ulsm, int ulsm_nob)
static int
echo_copyin_lsm (struct echo_device *ed, struct lov_stripe_md *lsm,
- void *ulsm, int ulsm_nob)
+ struct lov_stripe_md __user *ulsm, int ulsm_nob)
{
struct echo_client_obd *ec = ed->ed_ec;
+ struct lov_oinfo **p;
int i;
if (ulsm_nob < sizeof (*lsm))
@@ -1288,11 +1291,10 @@ echo_copyin_lsm (struct echo_device *ed, struct lov_stripe_md *lsm,
return -EINVAL;
- for (i = 0; i < lsm->lsm_stripe_count; i++) {
- if (copy_from_user(lsm->lsm_oinfo[i],
- ((struct lov_stripe_md *)ulsm)-> \
- lsm_oinfo[i],
- sizeof(lsm->lsm_oinfo[0])))
+ for (i = 0, p = lsm->lsm_oinfo; i < lsm->lsm_stripe_count; i++, p++) {
+ struct lov_oinfo __user *up;
+ if (get_user(up, ulsm->lsm_oinfo + i) ||
+ copy_from_user(*p, up, sizeof(struct lov_oinfo)))
return -EFAULT;
}
return 0;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
