[PATCH 1/1] lustre: Deletion of unnecessary checks before three function calls
by SF Markus Elfring
From: Markus Elfring <elfring(a)users.sourceforge.net>
Date: Tue, 2 Dec 2014 11:40:33 +0100
The functions free_ll_remote_perm(), free_rmtperm_hash() and iput() test
whether their argument is NULL and then return immediately.
Thus the test around their calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring(a)users.sourceforge.net>
---
drivers/staging/lustre/lustre/llite/remote_perm.c | 5 ++---
drivers/staging/lustre/lustre/llite/statahead.c | 3 +--
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/lustre/lustre/llite/remote_perm.c b/drivers/staging/lustre/lustre/llite/remote_perm.c
index c05a912..a581826 100644
--- a/drivers/staging/lustre/lustre/llite/remote_perm.c
+++ b/drivers/staging/lustre/lustre/llite/remote_perm.c
@@ -194,7 +194,7 @@ int ll_update_remote_perm(struct inode *inode, struct mdt_remote_perm *perm)
if (!lli->lli_remote_perms)
lli->lli_remote_perms = perm_hash;
- else if (perm_hash)
+ else
free_rmtperm_hash(perm_hash);
head = lli->lli_remote_perms + remote_perm_hashfunc(perm->rp_uid);
@@ -209,8 +209,7 @@ again:
continue;
if (tmp->lrp_fsgid != perm->rp_fsgid)
continue;
- if (lrp)
- free_ll_remote_perm(lrp);
+ free_ll_remote_perm(lrp);
lrp = tmp;
break;
}
diff --git a/drivers/staging/lustre/lustre/llite/statahead.c b/drivers/staging/lustre/lustre/llite/statahead.c
index 227854b..6ad9dd0 100644
--- a/drivers/staging/lustre/lustre/llite/statahead.c
+++ b/drivers/staging/lustre/lustre/llite/statahead.c
@@ -334,8 +334,7 @@ static void ll_sa_entry_put(struct ll_statahead_info *sai,
LASSERT(ll_sa_entry_unhashed(entry));
ll_sa_entry_cleanup(sai, entry);
- if (entry->se_inode)
- iput(entry->se_inode);
+ iput(entry->se_inode);
OBD_FREE(entry, entry->se_size);
atomic_dec(&sai->sai_cache_count);
--
2.1.3
6 years, 12 months
a bunch of lustre bugs...
by Al Viro
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(a)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(a)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(a)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(a)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
6 years, 12 months