[PATCH] xfs: fix memory exposure problems

[PATCH] xfs: fix memory exposure problems

[Darrick J. Wong]

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(-)

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;

Re: [PATCH] xfs: fix memory exposure problems

[Christoph Hellwig]

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>

Re: [PATCH] xfs: fix memory exposure problems

[Eric Sandeen]

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
>