On Nov 30, 2014, at 2:08 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> 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?
This is old testing code that was recently deleted from our development
tree. John, could you push your patch http://review.whamcloud.com/10139
upstream to Greg KH? It cleans up a bunch of ioctl cruft.
> 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.
This code was deleted in the same patch.
> 3) echo_copyin_lsm() has the same issues (both of them).
This needs the fix, thanks.
> 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.
We just started adding __user annotation to our code, and see that
"buffer" is annotated already, but it doesn't look like we have a
patch to fix this issue yet. Thanks for pointing it out.
> 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.
Looks like this needs copy_from_user() here and copy_to_user() on the
other side. Fortunately, Lustre isn't being run on embedded or other
strange systems w/o proper MMUs.
> 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...
You're right - ll_*fiemap are for older kernels (FIEMAP was first
added to the upstream kernel in 2.6.27, long after it was in Lustre)
but they aren't needed in the upstream kernel.
> 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
Cheers, Andreas
--
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
