diff mbox series

[6/6] xfs: don't crash the vfs on a garbage inline symlink

Message ID 153400173428.27471.504421086760762828.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfs-4.19: various fixes | expand

Commit Message

Darrick J. Wong Aug. 11, 2018, 3:35 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

The VFS routine that calls ->get_link blindly copies whatever's returned
into the user's buffer.  If we return a NULL pointer, the vfs will
crash on the null pointer.  Therefore, return -EFSCORRUPTED instead of
blowing up the kernel.

Reported-by: wen.xu@gatech.edu
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_iops.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Allison Henderson Aug. 12, 2018, 7:54 a.m. UTC | #1 
On 08/11/2018 08:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The VFS routine that calls ->get_link blindly copies whatever's returned
> into the user's buffer.  If we return a NULL pointer, the vfs will
> crash on the null pointer.  Therefore, return -EFSCORRUPTED instead of
> blowing up the kernel.
> 
> Reported-by: wen.xu@gatech.edu
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_iops.c |   10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 0ef5ad7fb851..26007a9db49d 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -471,8 +471,16 @@ xfs_vn_get_link_inline(
>   	struct inode		*inode,
>   	struct delayed_call	*done)
>   {
> +	char			*ptr;
> +
>   	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> -	return XFS_I(inode)->i_df.if_u1.if_data;
> +
> +	/*
> +	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
> +	 * if_data is junk.
> +	 */
> +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> +	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
>   }
>   
>   STATIC int
> 
Ok, looks fine.
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Christoph Hellwig Aug. 13, 2018, 7:23 a.m. UTC | #2 
>  	struct delayed_call	*done)
>  {
> +	char			*ptr;
> +
>  	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> -	return XFS_I(inode)->i_df.if_u1.if_data;
> +
> +	/*
> +	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
> +	 * if_data is junk.
> +	 */
> +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> +	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
>  }
>  
>  STATIC int

Please simplify this to:

>  	struct delayed_call	*done)
>  {
	char			*link = XFS_I(inode)->i_df.if_u1.if_data;

  	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> +	ptr = XFS_I(inode)->i_df.if_u1.if_data;

	if (!link)
		return ERR_PTR(-EFSCORRUPTED);
	return link;

But be honest I'd much rather fix this in the caller than every fs.
Can you send a patch to Al to do that instead?
Xu, Wen Aug. 19, 2018, 9:07 p.m. UTC | #3 
Hi Darrick,

Could I know what bugzilla bug I reported this patch corresponds to?

Thanks,
Wen

> On Aug 11, 2018, at 11:35 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The VFS routine that calls ->get_link blindly copies whatever's returned
> into the user's buffer.  If we return a NULL pointer, the vfs will
> crash on the null pointer.  Therefore, return -EFSCORRUPTED instead of
> blowing up the kernel.
> 
> Reported-by: wen.xu@gatech.edu
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/xfs_iops.c |   10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 0ef5ad7fb851..26007a9db49d 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -471,8 +471,16 @@ xfs_vn_get_link_inline(
> 	struct inode		*inode,
> 	struct delayed_call	*done)
> {
> +	char			*ptr;
> +
> 	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> -	return XFS_I(inode)->i_df.if_u1.if_data;
> +
> +	/*
> +	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
> +	 * if_data is junk.
> +	 */
> +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> +	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
> }
> 
> STATIC int
>
Darrick J. Wong Sept. 28, 2018, 12:31 a.m. UTC | #4 
On Mon, Aug 13, 2018 at 12:23:13AM -0700, Christoph Hellwig wrote:
> >  	struct delayed_call	*done)
> >  {
> > +	char			*ptr;
> > +
> >  	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> > -	return XFS_I(inode)->i_df.if_u1.if_data;
> > +
> > +	/*
> > +	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
> > +	 * if_data is junk.
> > +	 */
> > +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> > +	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
> >  }
> >  
> >  STATIC int
> 
> Please simplify this to:
> 
> >  	struct delayed_call	*done)
> >  {
> 	char			*link = XFS_I(inode)->i_df.if_u1.if_data;
> 
>   	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> > +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> 
> 	if (!link)
> 		return ERR_PTR(-EFSCORRUPTED);
> 	return link;
> 
> But be honest I'd much rather fix this in the caller than every fs.
> Can you send a patch to Al to do that instead?

Ok, I'll do that.

--D
diff mbox series

Patch

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0ef5ad7fb851..26007a9db49d 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -471,8 +471,16 @@  xfs_vn_get_link_inline(
 	struct inode		*inode,
 	struct delayed_call	*done)
 {
+	char			*ptr;
+
 	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
-	return XFS_I(inode)->i_df.if_u1.if_data;
+
+	/*
+	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
+	 * if_data is junk.
+	 */
+	ptr = XFS_I(inode)->i_df.if_u1.if_data;
+	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
 }
 
 STATIC int