From: Al Viro <viro@ZenIV.linux.org.uk>
To: Oleg Drokin <oleg.drokin@intel.com>
Cc: Andreas Dilger <andreas.dilger@intel.com>,
linux-fsdevel@vger.kernel.org, HPDD-discuss@ml01.01.org
Subject: a bunch of lustre bugs...
Date: Sun, 30 Nov 2014 21:08:08 +0000 [thread overview]
Message-ID: <20141130210808.GF29748@ZenIV.linux.org.uk> (raw)
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@zeniv.linux.org.uk>
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@zeniv.linux.org.uk>
---
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@zeniv.linux.org.uk>
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@zeniv.linux.org.uk>
---
.../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
next reply other threads:[~2014-11-30 21:08 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-30 21:08 Al Viro [this message]
2014-12-01 23:30 ` a bunch of lustre bugs Andreas Dilger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141130210808.GF29748@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=HPDD-discuss@ml01.01.org \
--cc=andreas.dilger@intel.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=oleg.drokin@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).