| Message ID | 20181215012259.28358-3-nbowler@draconx.ca (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Fixing xfs ioctls on x32 | expand |
On Fri, Dec 14, 2018 at 08:22:58PM -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_fsop_bulkreq > struct contains pointers and 32-bit integers so that matches the native > 32-bit layout, and that means the ioctl implementation goes into the > regular compat path on x32. > > However, the 'ubuffer' member of that struct in turn refers to either > struct xfs_inogrp or xfs_bstat (or an array of these). On x32, those > structures match the native 64-bit layout. The compat implementation > writes out the 32-bit version of these structures. This is not the > expected format for x32 userspace, causing problems. > > Fortunately the functions which actually output these xfs_inogrp and > xfs_bstat structures have an easy way to select which output format > is required, so we just need a little tweak to select the right format > on x32. > > Signed-off-by: Nick Bowler <nbowler@draconx.ca> > --- > fs/xfs/xfs_ioctl32.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > index 4c34efcbf7e8..1cdc75dca779 100644 > --- a/fs/xfs/xfs_ioctl32.c > +++ b/fs/xfs/xfs_ioctl32.c > @@ -241,6 +241,32 @@ xfs_compat_ioc_bulkstat( > int done; > int error; > > + /* > + * Output structure handling functions. Depending on the command, > + * either the xfs_bstat and xfs_inogrp structures are written out > + * to userpace memory via bulkreq.ubuffer. Normally the compat > + * functions and structure size are the correct ones to use ... > + */ > + 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()) { > + /* > + * ... but on x32 the input xfs_fsop_bulkreq has pointers > + * which must be handled in the "compat" (32-bit) way, while > + * the xfs_bstat and xfs_inogrp structures follow native 64- > + * bit layout convention. So adjust accordingly, otherwise > + * the data written out in compat layout will not match what > + * x32 userspace expects. > + */ > + inumbers_func = xfs_inumbers_fmt; > + bs_one_func = xfs_bulkstat_one; > + bs_one_size = sizeof(xfs_bstat_t); struct xfs_bstat, not xfs_bstat_t, because we're gradually getting rid of the structure typedefs. If I don't hear any other objections in the next day or so I'll fix this when I pull in this patch. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > + } > +#endif > + > /* done = 1 if there are more stats to get and if bulkstat */ > /* should be called again (unused here, but used in dmapi) */ > > @@ -272,15 +298,15 @@ xfs_compat_ioc_bulkstat( > > if (cmd == XFS_IOC_FSINUMBERS_32) { > error = xfs_inumbers(mp, &inlast, &count, > - bulkreq.ubuffer, xfs_inumbers_fmt_compat); > + bulkreq.ubuffer, inumbers_func); > } else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32) { > int res; > > - error = xfs_bulkstat_one_compat(mp, inlast, bulkreq.ubuffer, > - sizeof(compat_xfs_bstat_t), NULL, &res); > + error = bs_one_func(mp, inlast, bulkreq.ubuffer, > + bs_one_size, NULL, &res); > } else if (cmd == XFS_IOC_FSBULKSTAT_32) { > error = xfs_bulkstat(mp, &inlast, &count, > - xfs_bulkstat_one_compat, sizeof(compat_xfs_bstat_t), > + bs_one_func, bs_one_size, > bulkreq.ubuffer, &done); > } else > error = -EINVAL; > -- > 2.16.1 >
On 2018-12-17 Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Fri, Dec 14, 2018 at 08:22:58PM -0500, Nick Bowler wrote: [...] >> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c >> index 4c34efcbf7e8..1cdc75dca779 100644 >> --- a/fs/xfs/xfs_ioctl32.c >> +++ b/fs/xfs/xfs_ioctl32.c >> @@ -241,6 +241,32 @@ xfs_compat_ioc_bulkstat( >> int done; >> int error; >> >> + /* >> + * Output structure handling functions. Depending on the command, >> + * either the xfs_bstat and xfs_inogrp structures are written out >> + * to userpace memory via bulkreq.ubuffer. Normally the compat >> + * functions and structure size are the correct ones to use ... >> + */ >> + 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()) { >> + /* >> + * ... but on x32 the input xfs_fsop_bulkreq has pointers >> + * which must be handled in the "compat" (32-bit) way, while >> + * the xfs_bstat and xfs_inogrp structures follow native 64- >> + * bit layout convention. So adjust accordingly, otherwise >> + * the data written out in compat layout will not match what >> + * x32 userspace expects. >> + */ >> + inumbers_func = xfs_inumbers_fmt; >> + bs_one_func = xfs_bulkstat_one; >> + bs_one_size = sizeof(xfs_bstat_t); > > struct xfs_bstat, not xfs_bstat_t, because we're gradually getting rid > of the structure typedefs. If I don't hear any other objections in the > next day or so I'll fix this when I pull in this patch. Makes sense. I'll only change this on my end if I have to respin the series for some other reason then. I guess the sizeof(compat_xfs_bstat_t) just before this should be similarly changed as well? > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> Thanks, Nick
On Mon, Dec 17, 2018 at 03:06:43PM -0500, Nick Bowler wrote: > On 2018-12-17 Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Fri, Dec 14, 2018 at 08:22:58PM -0500, Nick Bowler wrote: > [...] > >> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > >> index 4c34efcbf7e8..1cdc75dca779 100644 > >> --- a/fs/xfs/xfs_ioctl32.c > >> +++ b/fs/xfs/xfs_ioctl32.c > >> @@ -241,6 +241,32 @@ xfs_compat_ioc_bulkstat( > >> int done; > >> int error; > >> > >> + /* > >> + * Output structure handling functions. Depending on the command, > >> + * either the xfs_bstat and xfs_inogrp structures are written out > >> + * to userpace memory via bulkreq.ubuffer. Normally the compat > >> + * functions and structure size are the correct ones to use ... > >> + */ > >> + 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()) { > >> + /* > >> + * ... but on x32 the input xfs_fsop_bulkreq has pointers > >> + * which must be handled in the "compat" (32-bit) way, while > >> + * the xfs_bstat and xfs_inogrp structures follow native 64- > >> + * bit layout convention. So adjust accordingly, otherwise > >> + * the data written out in compat layout will not match what > >> + * x32 userspace expects. > >> + */ > >> + inumbers_func = xfs_inumbers_fmt; > >> + bs_one_func = xfs_bulkstat_one; > >> + bs_one_size = sizeof(xfs_bstat_t); > > > > struct xfs_bstat, not xfs_bstat_t, because we're gradually getting rid > > of the structure typedefs. If I don't hear any other objections in the > > next day or so I'll fix this when I pull in this patch. > > Makes sense. I'll only change this on my end if I have to respin the > series for some other reason then. I guess the sizeof(compat_xfs_bstat_t) > just before this should be similarly changed as well? Yep. --D > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > Thanks, > Nick
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index 4c34efcbf7e8..1cdc75dca779 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -241,6 +241,32 @@ xfs_compat_ioc_bulkstat( int done; int error; + /* + * Output structure handling functions. Depending on the command, + * either the xfs_bstat and xfs_inogrp structures are written out + * to userpace memory via bulkreq.ubuffer. Normally the compat + * functions and structure size are the correct ones to use ... + */ + 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()) { + /* + * ... but on x32 the input xfs_fsop_bulkreq has pointers + * which must be handled in the "compat" (32-bit) way, while + * the xfs_bstat and xfs_inogrp structures follow native 64- + * bit layout convention. So adjust accordingly, otherwise + * the data written out in compat layout will not match what + * x32 userspace expects. + */ + inumbers_func = xfs_inumbers_fmt; + bs_one_func = xfs_bulkstat_one; + bs_one_size = sizeof(xfs_bstat_t); + } +#endif + /* done = 1 if there are more stats to get and if bulkstat */ /* should be called again (unused here, but used in dmapi) */ @@ -272,15 +298,15 @@ xfs_compat_ioc_bulkstat( if (cmd == XFS_IOC_FSINUMBERS_32) { error = xfs_inumbers(mp, &inlast, &count, - bulkreq.ubuffer, xfs_inumbers_fmt_compat); + bulkreq.ubuffer, inumbers_func); } else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32) { int res; - error = xfs_bulkstat_one_compat(mp, inlast, bulkreq.ubuffer, - sizeof(compat_xfs_bstat_t), NULL, &res); + error = bs_one_func(mp, inlast, bulkreq.ubuffer, + bs_one_size, NULL, &res); } else if (cmd == XFS_IOC_FSBULKSTAT_32) { error = xfs_bulkstat(mp, &inlast, &count, - xfs_bulkstat_one_compat, sizeof(compat_xfs_bstat_t), + bs_one_func, bs_one_size, bulkreq.ubuffer, &done); } else error = -EINVAL;
The bulkstat family of ioctls are problematic on x32, because there is a mixup of native 32-bit and 64-bit conventions. The xfs_fsop_bulkreq struct contains pointers and 32-bit integers so that matches the native 32-bit layout, and that means the ioctl implementation goes into the regular compat path on x32. However, the 'ubuffer' member of that struct in turn refers to either struct xfs_inogrp or xfs_bstat (or an array of these). On x32, those structures match the native 64-bit layout. The compat implementation writes out the 32-bit version of these structures. This is not the expected format for x32 userspace, causing problems. Fortunately the functions which actually output these xfs_inogrp and xfs_bstat structures have an easy way to select which output format is required, so we just need a little tweak to select the right format on x32. Signed-off-by: Nick Bowler <nbowler@draconx.ca> --- fs/xfs/xfs_ioctl32.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)