Re: [PATCH 2/2] t_attr_corruption: fix this yet again

[Allison Henderson <allison.henderson@oracle.com>]

On 2/25/19 7:35 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Jeff Moyer pointed out that 'security.evm' actually has an expected
> value format, which breaks the test if EVM is enabled.  It turns out
> that the 'security.evm' setxattr call in the original syzkaller report
> was a total red herring, as this bug can be reproduced without it.
> 
> Fix the test case to do the minimum amount of work needed to reproduce
> the corruption.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   src/t_attr_corruption.c |   28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> 
> diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
> index f26611f9..9101024e 100644
> --- a/src/t_attr_corruption.c
> +++ b/src/t_attr_corruption.c
> @@ -3,17 +3,21 @@
>    * Copyright (C) 2019 Oracle.  All Rights Reserved.
>    * Author: Darrick J. Wong <darrick.wong@oracle.com>
>    *
> - * Test program to tickle a use-after-free bug in xfs.
> + * XFS had a memory corruption bug in its handling of the POSIX ACL attribute
> + * names during a listxattr call.
>    *
> - * XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> - * listxattr buffer space while trying to store the name
> - * "system.posix_acl_access" and then corrupts memory by not checking the
> - * seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into the
> - * buffer as well.
> + * On IRIX, file ACLs were stored under the name "trusted.SGI_ACL_FILE",
> + * whereas on Linux the name is "system.posix_acl_access".  In order to
> + * maintain compatibility with old filesystems, XFS internally continues to
> + * use the old SGI_ACL_FILE name on disk and remap the new name whenever it
> + * sees it.
>    *
> - * In order to tickle the bug in a user visible way we must have already put a
> - * name in the buffer, so we take advantage of the fact that "security.evm"
> - * sorts before "system.posix_acl_access" to make sure this happens.
> + * In order to make this magic happen, XFS' listxattr implementation will emit
> + * first the Linux name and then the on-disk name.  Unfortunately, it doesn't
> + * correctly check the buffer length, so if the buffer is large enough to fit
> + * the on-disk name but not large enough to fit the Linux name, we screw up
> + * the buffer position accounting while trying to emit the Linux name and then
> + * corrupt memory when we try to emit the on-disk name.

Ok, thanks for the explanation!  You can add my review:

Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

>    *
>    * If we trigger the bug, the program will print the garbled string
>    * "rusted.SGI_ACL_FILE".  If the bug is fixed, the flistxattr call returns
> @@ -76,11 +80,7 @@ int main(int argc, char *argv[])
>   	if (ret)
>   		die("set posix acl");
>   
> -	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
> -	if (ret)
> -		die("set evm");
> -
> -	sz = flistxattr(fd, buf, 30);
> +	sz = flistxattr(fd, buf, 20);
>   	if (sz < 0)
>   		die("list attr");
>   
>