| Message ID | 20170401012934.GI4864@birch.djwong.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 31, 2017 at 06:29:34PM -0700, Darrick J. Wong wrote: > Fix a couple of memory exposure problems in the getbmap implementation > where we copy too much header data from userspace, and a second problem > in inumbers where we allocate an array of structures with holes, fail to > zero the holes, then blindly copy the kernel memory contents into > userspace. This should be two different patches. > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1547,10 +1547,10 @@ xfs_ioc_getbmap( > unsigned int cmd, > void __user *arg) > { > - struct getbmapx bmx; > + struct getbmapx bmx = {0}; missing spaces around the zero. > int error; > > - if (copy_from_user(&bmx, arg, sizeof(struct getbmapx))) > + if (copy_from_user(&bmx, arg, sizeof(struct getbmap))) > return -EFAULT; This really needs a comment on how struct getbmap is a strict subset of struct getbmapx. Or even better don't use getbmap at all and use offsetoff on struct getbmapx. > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c > index e775f78..55642cd 100644 > --- a/fs/xfs/xfs_itable.c > +++ b/fs/xfs/xfs_itable.c > @@ -584,7 +584,7 @@ xfs_inumbers( > return error; > > bcount = MIN(left, (int)(PAGE_SIZE / sizeof(*buffer))); > - buffer = kmem_alloc(bcount * sizeof(*buffer), KM_SLEEP); > + buffer = kmem_zalloc(bcount * sizeof(*buffer), KM_SLEEP); This looks fine: Reviewed-by: Christoph Hellwig <hch@lst.de> -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/1/17 2:04 AM, Christoph Hellwig wrote: > On Fri, Mar 31, 2017 at 06:29:34PM -0700, Darrick J. Wong wrote: >> Fix a couple of memory exposure problems in the getbmap implementation >> where we copy too much header data from userspace, and a second problem >> in inumbers where we allocate an array of structures with holes, fail to >> zero the holes, then blindly copy the kernel memory contents into >> userspace. > > This should be two different patches. > >> --- a/fs/xfs/xfs_ioctl.c >> +++ b/fs/xfs/xfs_ioctl.c >> @@ -1547,10 +1547,10 @@ xfs_ioc_getbmap( >> unsigned int cmd, >> void __user *arg) >> { >> - struct getbmapx bmx; >> + struct getbmapx bmx = {0}; > > missing spaces around the zero. > >> int error; >> >> - if (copy_from_user(&bmx, arg, sizeof(struct getbmapx))) >> + if (copy_from_user(&bmx, arg, sizeof(struct getbmap))) >> return -EFAULT; > > This really needs a comment on how struct getbmap is a strict subset > of struct getbmapx. Or even better don't use getbmap at all > and use offsetoff on struct getbmapx. Agreed, was going to make the same observation. -Eric >> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c >> index e775f78..55642cd 100644 >> --- a/fs/xfs/xfs_itable.c >> +++ b/fs/xfs/xfs_itable.c >> @@ -584,7 +584,7 @@ xfs_inumbers( >> return error; >> >> bcount = MIN(left, (int)(PAGE_SIZE / sizeof(*buffer))); >> - buffer = kmem_alloc(bcount * sizeof(*buffer), KM_SLEEP); >> + buffer = kmem_zalloc(bcount * sizeof(*buffer), KM_SLEEP); > > This looks fine: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index b0250ed..14c2301 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1547,10 +1547,10 @@ xfs_ioc_getbmap( unsigned int cmd, void __user *arg) { - struct getbmapx bmx; + struct getbmapx bmx = {0}; int error; - if (copy_from_user(&bmx, arg, sizeof(struct getbmapx))) + if (copy_from_user(&bmx, arg, sizeof(struct getbmap))) return -EFAULT; if (bmx.bmv_count < 2) diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c index e775f78..55642cd 100644 --- a/fs/xfs/xfs_itable.c +++ b/fs/xfs/xfs_itable.c @@ -584,7 +584,7 @@ xfs_inumbers( return error; bcount = MIN(left, (int)(PAGE_SIZE / sizeof(*buffer))); - buffer = kmem_alloc(bcount * sizeof(*buffer), KM_SLEEP); + buffer = kmem_zalloc(bcount * sizeof(*buffer), KM_SLEEP); do { struct xfs_inobt_rec_incore r; int stat;
Fix a couple of memory exposure problems in the getbmap implementation where we copy too much header data from userspace, and a second problem in inumbers where we allocate an array of structures with holes, fail to zero the holes, then blindly copy the kernel memory contents into userspace. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- fs/xfs/xfs_ioctl.c | 4 ++-- fs/xfs/xfs_itable.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html