git: ebbc3140ca0d - main - truss: Decode correctly 64bits arguments on 32bits arm.

[Jessica Clarke]

On 23 Sep 2021, at 00:05, Olivier Houchard <cognet at FreeBSD.org> wrote:
> 
> The branch main has been updated by cognet:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=ebbc3140ca0d7eee154f7a67ccdae7d3d88d13fd
> 
> commit ebbc3140ca0d7eee154f7a67ccdae7d3d88d13fd
> Author:     Olivier Houchard <cognet at FreeBSD.org>
> AuthorDate: 2021-09-22 22:45:42 +0000
> Commit:     Olivier Houchard <cognet at FreeBSD.org>
> CommitDate: 2021-09-22 23:04:16 +0000
> 
>    truss: Decode correctly 64bits arguments on 32bits arm.
> 
>    When decoding 32bits arm syscall, make sure we account for the padding when
>    decoding 64bits args. Do it too when using a 64bits truss on a 32bits binary.
> 
>    MFC After:      1 week
>    PR:             256199
> ---
> usr.bin/truss/syscalls.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/usr.bin/truss/syscalls.c b/usr.bin/truss/syscalls.c
> index f7657f30b583..9cd53e71cc9b 100644
> --- a/usr.bin/truss/syscalls.c
> +++ b/usr.bin/truss/syscalls.c
> @@ -792,11 +792,14 @@ print_mask_arg32(bool (*decoder)(FILE *, uint32_t, uint32_t *), FILE *fp,
>  * decoding arguments.
>  */
> static void
> -quad_fixup(struct syscall_decode *sc)
> +quad_fixup(struct procabi *abi, struct syscall_decode *sc)
> {
> 	int offset, prev;
> 	u_int i;
> 
> +#ifndef __aarch64__
> +	(void)abi;
> +#endif
> 	offset = 0;
> 	prev = -1;
> 	for (i = 0; i < sc->nargs; i++) {
> @@ -810,17 +813,20 @@ quad_fixup(struct syscall_decode *sc)
> 		switch (sc->args[i].type & ARG_MASK) {
> 		case Quad:
> 		case QuadHex:
> -#ifdef __powerpc__
> +#if defined(__powerpc__) || defined(__arm__) || defined(__aarch64__)
> 			/*
> -			 * 64-bit arguments on 32-bit powerpc must be
> +			 * 64-bit arguments on 32-bit powerpc and arm must be
> 			 * 64-bit aligned.  If the current offset is
> 			 * not aligned, the calling convention inserts
> 			 * a 32-bit pad argument that should be skipped.
> 			 */
> -			if (sc->args[i].offset % 2 == 1) {
> -				sc->args[i].offset++;
> -				offset++;
> -			}
> +#ifdef __aarch64__
> +			if (abi->pointer_size == sizeof(uint32_t))

Why? The caller already checks this, it doesn’t need to be checked
again. You even had to update that caller to pass the ABI and the patch
clearly shows the check there.

Also, please put changes like this up for review in future,
arichardson@ did a bunch of work getting non-native ABI support into
truss with input from myself, so if you find you need anything other
than things like adding to ifdefs then that’s news to us because we’ve
tested it with freebsd32, and downstream in CheriBSD with our added
freebsd64 ABI (which is what you think of as normal 64-bit, but we add
a new ABI for memory safety that is the native ABI, so we exercise even
more weird cases than upstream can).

Please revert everything other than adding to the existing ifdef and
updating the comment.

Jess

> +#endif
> +				if (sc->args[i].offset % 2 == 1) {
> +					sc->args[i].offset++;
> +					offset++;
> +				}
> #endif
> 			offset++;
> 		default:
> @@ -854,7 +860,7 @@ add_syscall(struct procabi *abi, u_int number, struct syscall *sc)
> 	 *  procabi instead.
> 	 */
> 	if (abi->pointer_size == 4)
> -		quad_fixup(&sc->decode);
> +		quad_fixup(abi, &sc->decode);
> 
> 	if (number < nitems(abi->syscalls)) {
> 		assert(abi->syscalls[number] == NULL);