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

Allison Henderson
2019-02-26T20:03:47Z

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");
>
>