Re: [RFC PATCH 1/2] xfs: Fix bulkstat compat ioctls on x32 userspace.

[Brian Foster <bfoster@redhat.com>]

On Thu, Dec 13, 2018 at 12:44:16PM -0500, Nick Bowler wrote:
> On 2018-12-13, Brian Foster <bfoster@redhat.com> wrote:
> > On Wed, Dec 12, 2018 at 08:29:59PM -0500, Nick Bowler wrote:
> >> The bulkstat family of ioctls are problematic on x32, because there is
> >> a mixup of native 32-bit and 64-bit conventions: the xfs_bulkstat struct
> >> contains pointers and 32-bit integers so that fits the native 32-bit
> >> layout fine.  However, one of those pointers is subsequently used to
> >> refer to one of several structs which, on x32, all follow the native
> >> 64-bit way.
> >>
> >> Fortunately the pointer chasing seems to end there, and the functions to
> >> deal with this abstract things pretty well.  We just need a little tweak
> >> to pass the right formatting function if called from x32 mode.
> >>
> > > Could you be a bit more specific on the problem? What
> > pointers/structures are problematic? What exactly is "the xfs_bulkstat
> > struct?"
> 
> A mistake.  I meant struct xfs_fsop_bulkreq; I'll fix the commit message.
> 
> The problem is:
> 
>   - xfs_fsop_bulkreq on x32 matches IA-32 layout on x32, so the
>     ioctl cmd number matches and the implementation calls
>     xfs_compat_ioc_bulkstat.
> 

I assume that this is because xfs_fsop_bulkreq includes pointers, which
is where x32 and x86_64 actually start to differ..? So in this
particular case, the two ioctl() structs actually are different between
x32 and x86_64.

>   - The 'ubuffer' pointer in that structure refers to either struct
>     xfs_bstat or struct xfs_inogrp.  On x32 both of these structures
>     match the native 64-bit layout; the compat path writes out the
>     IA-32 layout which is incorrectly formatted for x32 userspace.
> 

Ok.

> The proposed solution is:
> 
>   - Use in_x32_syscall to distinguish the IA-32 and x32 cases, the
>     functions which do this have a easy way to select which output
>     format is required, so we just need to pick the right one on x32.
> 

A little hairy, but it makes more sense now. Thanks.

> >> ---
> >>  fs/xfs/xfs_ioctl32.c | 25 +++++++++++++++++++++----
> >>  1 file changed, 21 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> >> index fba115f4103a..6a759c0ffb54 100644
> >> --- a/fs/xfs/xfs_ioctl32.c
> >> +++ b/fs/xfs/xfs_ioctl32.c
> >> @@ -241,6 +241,23 @@ xfs_compat_ioc_bulkstat(
> >>  	int			done;
> >>  	int			error;
> >>
> >> +	/*
> >> +	 * These functions and size are used later to handle individual
> >> +	 * entries; x32 is annoying and needs different functions.
> >> +	 */
> >
> > Same here, this describes the change but doesn't help me understand the
> > problem.
> 
> I'll think about a better way to write this comment.
> 

I'd suggest to use parts of what you've just described above.

Brian

> >> +	inumbers_fmt_pf inumbers_func = xfs_inumbers_fmt_compat;
> >> +	bulkstat_one_pf	bs_one_func = xfs_bulkstat_one_compat;
> >> +	size_t bs_one_size = sizeof(compat_xfs_bstat_t);
> >> +
> >> +#ifdef CONFIG_X86_X32
> >> +	if (in_x32_syscall()) {
> >> +		/* x32 matches native amd64 bstat and inogrp layout */
> >> +		inumbers_func = xfs_inumbers_fmt;
> >> +		bs_one_func = xfs_bulkstat_one;
> >> +		bs_one_size = sizeof(xfs_bstat_t);
> >> +	}
> >> +#endif
> >> +
> >
> > Would this be necessary if the higher level x32 code called into
> > xfs_ioc_bulkstat() instead of the compat variant, or is there some
> > other reason x32 wouldn't work through that path?
> 
> The xfs_compat_ioc_bulkstat function does two things: it reads in the
> xfs_fsop_bulkreq structure (matches ia-32 layout on x32), then it writes
> out the xfs_inorgp or xfs_bstat structures depending on what operation
> was requested; both of these structures match amd64 layout on x32.
> 
> So the goal of the change is to adjust the second behaviour only.
> 
> Cheers,
>   Nick