Re: [PATCH v2 1/2] futex: futex_wake_op, do not fail on invalid op

[Jiri Slaby]

Hi,

is this OK/going to be applied to 4.14 yet or postponed to 4.15-rc?

On 10/23/2017, 01:41 PM, Jiri Slaby wrote:
> In 30d6e0a4190d ("futex: Remove duplicated code and fix undefined
> behaviour"), I let FUTEX_WAKE_OP to fail on invalid op. Namely when
> op should be considered as shift and the shift is out of range (< 0 or
>> 31).
> 
> But strace's test suite does this madness:
> futex(0x7fabd78bcffc, 0x5, 0xfacefeed, 0xb, 0x7fabd78bcffc, 0xa0caffee);
> futex(0x7fabd78bcffc, 0x5, 0xfacefeed, 0xb, 0x7fabd78bcffc, 0xbadfaced);
> futex(0x7fabd78bcffc, 0x5, 0xfacefeed, 0xb, 0x7fabd78bcffc, 0xffffffff);
> 
> When I pick the first 0xa0caffee, it decodes as:
> 0x80000000 & 0xa0caffee: oparg is shift
> 0x70000000 & 0xa0caffee: op is FUTEX_OP_OR
> 0x0f000000 & 0xa0caffee: cmp is FUTEX_OP_CMP_EQ
> 0x00fff000 & 0xa0caffee: oparg is sign-extended 0xcaf = -849
> 0x00000fff & 0xa0caffee: cmparg is sign-extended 0xfee = -18
> 
> That means the op tries to do this:
>   (futex |= (1 << (-849))) == -18
> which is completely bogus. The new check of op in the code is:
>         if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
>                 if (oparg < 0 || oparg > 31)
>                         return -EINVAL;
>                 oparg = 1 << oparg;
>         }
> 
> which results obviously in the "Invalid argument" errno:
> ----8<--------8<--------8<--------8<--------8<----
> FAIL: futex
> ===========
> 
> futex(0x7fabd78bcffc, 0x5, 0xfacefeed, 0xb, 0x7fabd78bcffc, 0xa0caffee) = -1: Invalid argument
> futex.test: failed test: ../futex failed with code 1
> ----8<--------8<--------8<--------8<--------8<----
> 
> So let us soften the failure to print only a (ratelimited) message,
> crop the value and continue as if it were right. When userspace keeps
> up, we can switch this to return -EINVAL again.
> 
> [v2]
> Do not return 0 immediatelly, proceed with the cropped value.
> 
> Fixes: 30d6e0a4190d ("futex: Remove duplicated code and fix undefined behaviour")
> Signed-off-by: Jiri Slaby <jslaby@xxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Darren Hart <dvhart@xxxxxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> ---
>  kernel/futex.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 0518a0bfc746..0d638f008bb1 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1570,8 +1570,16 @@ static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
>  	int oldval, ret;
>  
>  	if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
> -		if (oparg < 0 || oparg > 31)
> -			return -EINVAL;
> +		if (oparg < 0 || oparg > 31) {
> +			char comm[sizeof(current->comm)];
> +			/*
> +			 * kill this print and return -EINVAL when userspace
> +			 * is sane again
> +			 */
> +			pr_info_ratelimited("futex_wake_op: %s tries to shift op by %d; fix this program\n",
> +					get_task_comm(comm, current), oparg);
> +			oparg &= 31;
> +		}
>  		oparg = 1 << oparg;
>  	}
>  
> 


-- 
js
suse labs