[PATCH] time: compat settimeofday: Validate the values of tv from user

[PATCH] time: compat settimeofday: Validate the values of tv from user

[zhengbin]

Similar to commit 6ada1fc0e1c4
("time: settimeofday: Validate the values of tv from user"),
an unvalidated user input is multiplied by a constant, which can result
in an undefined behaviour for large values. While this is validated
later, we should avoid triggering undefined behaviour.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 kernel/time/time.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/time/time.c b/kernel/time/time.c
index 7f7d691..5c54ca6 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -251,6 +251,10 @@ COMPAT_SYSCALL_DEFINE2(settimeofday, struct old_timeval32 __user *, tv,
 	if (tv) {
 		if (compat_get_timeval(&user_tv, tv))
 			return -EFAULT;
+
+		if (!timeval_valid(&user_tv))
+			return -EINVAL;
+
 		new_ts.tv_sec = user_tv.tv_sec;
 		new_ts.tv_nsec = user_tv.tv_usec * NSEC_PER_USEC;
 	}
--
2.7.4

Re: [PATCH] time: compat settimeofday: Validate the values of tv from user

[Thomas Gleixner]

Zhengbin,

On Fri, 5 Jul 2019, zhengbin wrote:

> Similar to commit 6ada1fc0e1c4
> ("time: settimeofday: Validate the values of tv from user"),
> an unvalidated user input is multiplied by a constant, which can result
> in an undefined behaviour for large values. While this is validated
> later, we should avoid triggering undefined behaviour.

I surely agree with the patch, but the argument that this is validated
later and we just should avoid UB in general is just wrong.

For a wide range of negative tv_usec values the multiplication overflow
turns them in positive numbers. So the 'validated later' is not catching
the invalid input.

So 'should avoid ....' is just the wrong argument here.

Validation _is_ required before the multiplication so UB won't turn an
invalid value into a valid one.

Thanks,

	tglx

Re: [PATCH] time: compat settimeofday: Validate the values of tv from user

[zhengbin (A)]

On 2019/7/5 20:14, Thomas Gleixner wrote:
> Zhengbin,
>
> On Fri, 5 Jul 2019, zhengbin wrote:
>
>> Similar to commit 6ada1fc0e1c4
>> ("time: settimeofday: Validate the values of tv from user"),
>> an unvalidated user input is multiplied by a constant, which can result
>> in an undefined behaviour for large values. While this is validated
>> later, we should avoid triggering undefined behaviour.
> I surely agree with the patch, but the argument that this is validated
> later and we just should avoid UB in general is just wrong.
>
> For a wide range of negative tv_usec values the multiplication overflow
> turns them in positive numbers. So the 'validated later' is not catching
> the invalid input.
>
> So 'should avoid ....' is just the wrong argument here.
>
> Validation _is_ required before the multiplication so UB won't turn an
> invalid value into a valid one.
>
> Thanks,
>
> 	tglx
Strongly agree with this, I send a v2 patch, modify the comment?
>
> .
>