[PATCH v2] xfs: fix kernel memory exposure problems

[PATCH v2] xfs: fix kernel memory exposure problems

[Darrick J. Wong]

Fix a memory exposure problems in inumbers where we allocate an array of
structures with holes, fail to zero the holes, then blindly copy the
kernel memory contents (junk and all) into userspace.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
v2: split patches
---
 fs/xfs/xfs_itable.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 2a6d9b1..26d67ce 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -583,7 +583,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;

[PATCH] xfs: fix over-copying of getbmap parameters from userspace

[Darrick J. Wong]

In xfs_ioc_getbmap, we should only copy the fields of struct getbmap
from userspace, or else we end up copying random stack contents into the
kernel.  struct getbmap is a strict subset of getbmapx, so a partial
structure copy should work fine.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 2fd7fdf..e44dde7 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1543,10 +1543,14 @@ 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)))
+	/*
+	 * struct getbmap is a strict subset of struct getbmapx,
+	 * so using a straight memory copy should work fine.
+	 */
+	if (copy_from_user(&bmx, arg, offsetof(struct getbmapx, bmv_iflags)))
 		return -EFAULT;
 
 	if (bmx.bmv_count < 2)

Re: [PATCH] xfs: fix over-copying of getbmap parameters from userspace

[Christoph Hellwig]

> +	 * so using a straight memory copy should work fine.

I'd skip this half of the comment.

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] xfs: fix over-copying of getbmap parameters from userspace

[Darrick J. Wong]

On Mon, Apr 03, 2017 at 11:57:02AM -0700, Christoph Hellwig wrote:
> > +	 * so using a straight memory copy should work fine.
> 
> I'd skip this half of the comment.

Will do.

> Otherwise 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

Re: [PATCH v2] xfs: fix kernel memory exposure problems

[Darrick J. Wong]

On Mon, Apr 03, 2017 at 10:34:30AM -0700, Darrick J. Wong wrote:
> Fix a memory exposure problems in inumbers where we allocate an array of
> structures with holes, fail to zero the holes, then blindly copy the
> kernel memory contents (junk and all) into userspace.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

BTW, I intend to send this patch (though not the getbmap patch) for 4.11
since kernel memory exposure is usually treated as a security problem.

--D

> ---
> v2: split patches
> ---
>  fs/xfs/xfs_itable.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 2a6d9b1..26d67ce 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -583,7 +583,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;
> --
> 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

Re: [PATCH v2] xfs: fix kernel memory exposure problems

[Eric Sandeen]

On 4/3/17 2:42 PM, Darrick J. Wong wrote:
> On Mon, Apr 03, 2017 at 10:34:30AM -0700, Darrick J. Wong wrote:
>> Fix a memory exposure problems in inumbers where we allocate an array of
>> structures with holes, fail to zero the holes, then blindly copy the
>> kernel memory contents (junk and all) into userspace.
>>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> BTW, I intend to send this patch (though not the getbmap patch) for 4.11
> since kernel memory exposure is usually treated as a security problem.

I agree with that plan; if this change isn't safe I don't know what
is ...

-Eric

> --D
> 
>> ---
>> v2: split patches
>> ---
>>  fs/xfs/xfs_itable.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
>> index 2a6d9b1..26d67ce 100644
>> --- a/fs/xfs/xfs_itable.c
>> +++ b/fs/xfs/xfs_itable.c
>> @@ -583,7 +583,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;
>> --
>> 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
> 

Re: [PATCH v2] xfs: fix kernel memory exposure problems

[Christoph Hellwig]

On Mon, Apr 03, 2017 at 03:06:42PM -0500, Eric Sandeen wrote:
> On 4/3/17 2:42 PM, Darrick J. Wong wrote:
> > On Mon, Apr 03, 2017 at 10:34:30AM -0700, Darrick J. Wong wrote:
> >> Fix a memory exposure problems in inumbers where we allocate an array of
> >> structures with holes, fail to zero the holes, then blindly copy the
> >> kernel memory contents (junk and all) into userspace.
> >>
> >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> > BTW, I intend to send this patch (though not the getbmap patch) for 4.11
> > since kernel memory exposure is usually treated as a security problem.
> 
> I agree with that plan; if this change isn't safe I don't know what
> is ...

Yes.  Please send this for 4.11-rc.