Re: [patch 3/3] Fix XFS_IOC_FSBULKSTAT{,_SINGLE} and XFS_IOC_FSINUMBERS in compat mode

Arnd Bergmann
2007-05-31T07:07:03Z

On Wednesday 30 May 2007, Michal Marek wrote:
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl32.c
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_ioctl32.c
> @@ -109,35 +109,249 @@ STATIC unsigned long xfs_ioctl32_geom_v1
> return (unsigned long)p;
> }
>
> -#else
> +typedef struct xfs_inogrp32 {
> + __u64 xi_startino; /* starting inode number */
> + __s32 xi_alloccount; /* # bits set in allocmask */
> + __u64 xi_allocmask; /* mask of allocated inodes */
> +} __attribute__((packed)) xfs_inogrp32_t;

__attribute__((packed)) isn't entirely correct here. You don't really
want to have the whole structure to have byte alignment, you only
want to reduce the alignment o fthe 64 bit members to 32 bit.

It would be more appropriate to define a separate type

#if defined(__x86_64__) || defined(__ia64__)
typedef unsigned long long __compat_u64 __attribute__((aligned(4)));
#else
typedef unsigned long long __compat_u64;
#endif

and use that in the data structures.

> +STATIC int xfs_inogrp_store_compat(
> + xfs_inogrp32_t __user *p32,
> + xfs_inogrp_t __user *p)
> +{
> +#define copy(memb) copy_in_user(&p32->memb, &p->memb, sizeof(p32->memb))
> + if (copy(xi_startino) ||
> + copy(xi_alloccount) ||
> + copy(xi_allocmask))
> + return -EFAULT;
> + return 0;
> +#undef copy
> +}

Your copy() operation looks really dangerous, it will break as soon as someone
tries to use it on a member that is actually variable length, like a pointer.
A better way would be

#define move_user(p32, p64, memb) ({ \
typeof(p32->memb) data; \
get_user(data, &p64->memb) || \
put_user(data, &p32->memb); \
})

Actually, even better would be not to use the compat_alloc_userspace trick
at all, but to just interpret the 32 bit data structure directly in the
implementation instead of converting it to the 64 bit structure, whereever
that's possible.