[PATCH] xfs: initialize the shortform attr header padding entry

[PATCH] xfs: initialize the shortform attr header padding entry

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Don't leak kernel memory contents into the shortform attr fork.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 8623c815164a..e1a3d225a77d 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -656,6 +656,7 @@ xfs_attr_shortform_create(
 	hdr = (xfs_attr_sf_hdr_t *)ifp->if_u1.if_data;
 	hdr->count = 0;
 	hdr->totsize = cpu_to_be16(sizeof(*hdr));
+	hdr->padding = 0;
 	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_ADATA);
 }
 

Re: [PATCH] xfs: initialize the shortform attr header padding entry

[Eric Sandeen]

On 8/25/20 3:28 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't leak kernel memory contents into the shortform attr fork.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I noticed this too, thanks.

thought I wonder if 

a) others lurk and
b) if we should just be memsetting if_data to zero to avoid the need
   to carefully initialize all of everything always?

Anyway it fixes the problem we noticed so

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 8623c815164a..e1a3d225a77d 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -656,6 +656,7 @@ xfs_attr_shortform_create(
>  	hdr = (xfs_attr_sf_hdr_t *)ifp->if_u1.if_data;
>  	hdr->count = 0;
>  	hdr->totsize = cpu_to_be16(sizeof(*hdr));
> +	hdr->padding = 0;
>  	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_ADATA);
>  }
>  
> 

Re: [PATCH] xfs: initialize the shortform attr header padding entry

[Dave Chinner]

On Tue, Aug 25, 2020 at 01:28:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't leak kernel memory contents into the shortform attr fork.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 8623c815164a..e1a3d225a77d 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -656,6 +656,7 @@ xfs_attr_shortform_create(
>  	hdr = (xfs_attr_sf_hdr_t *)ifp->if_u1.if_data;
>  	hdr->count = 0;
>  	hdr->totsize = cpu_to_be16(sizeof(*hdr));
> +	hdr->padding = 0;

memset(hdr, 0, sizeof(*hdr)), please.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: initialize the shortform attr header padding entry

[Christoph Hellwig]

On Tue, Aug 25, 2020 at 03:48:13PM -0500, Eric Sandeen wrote:
> On 8/25/20 3:28 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Don't leak kernel memory contents into the shortform attr fork.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I noticed this too, thanks.
> 
> thought I wonder if 
> 
> a) others lurk and
> b) if we should just be memsetting if_data to zero to avoid the need
>    to carefully initialize all of everything always?
> 
> Anyway it fixes the problem we noticed so

Yes, I think fully clearing the thing would be better.  We can do
a memset for now, but splitting the non-existent data case out of
xfs_idata_realloc into a new helper that does kzmalloc sounds best
long term.