Re: [PATCH] ipv6: fix signedness of tmp_prefered_lft underflow check

[David Miller]

From: Jiri Bohac 
Date: Wed, 19 Oct 2016 15:16:36 +0200

> The purpose was to guard against the user updating the
> temp_prefered_lft sysctl after this:
> 
> max_desync_factor = min_t(__u32,
>   idev->cnf.max_desync_factor,
>   idev->cnf.temp_prefered_lft - 
> regen_advance);
> 
> but before this:
> 
>   tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
>   idev->desync_factor;

That's a different problem.

Read the sysctl values of interest into local variables using
READ_ONCE() before the calculations, that way the situation your
describe is impossible.

Re: [PATCH] ipv6: fix signedness of tmp_prefered_lft underflow check

[Jiri Bohac]

Hi,

On Tue, Oct 18, 2016 at 02:25:25PM -0400, David Miller wrote:
> Does the check make any sense at all?  I'd say just remove it.

The purpose was to guard against the user updating the
temp_prefered_lft sysctl after this:

max_desync_factor = min_t(__u32,
  idev->cnf.max_desync_factor,
  idev->cnf.temp_prefered_lft - regen_advance);

but before this:

tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
idev->desync_factor;


With enough bad luck, tmp_prefered_lft could underflow and the resulting
preferred lifetime could be almost infinity.

On the other hand, with this check, this situation will result
with the temporary address not being created at all, which might
be even worse. So if you prefer it, just drop the check.
Patch in a follow-up e-mail.

Thanks,

-- 
Jiri Bohac 
SUSE Labs, SUSE CZ

Re: [PATCH] ipv6: fix signedness of tmp_prefered_lft underflow check

[David Miller]

From: Jiri Bohac 
Date: Tue, 18 Oct 2016 17:01:54 +0200

> Commit 76506a986dc31394fd1f2741db037d29c7e57843 (IPv6: fix
> DESYNC_FACTOR) introduced a buggy check for underflow of
> tmp_prefered_lft. tmp_prefered_lft is unsigned, so the condition
> is always false.
> 
> Signed-off-by: Jiri Bohac 
> Reported-by: Julia Lawall 
> Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")

Does the check make any sense at all?  I'd say just remove it.

[PATCH] ipv6: fix signedness of tmp_prefered_lft underflow check

[Jiri Bohac]

Commit 76506a986dc31394fd1f2741db037d29c7e57843 (IPv6: fix
DESYNC_FACTOR) introduced a buggy check for underflow of
tmp_prefered_lft. tmp_prefered_lft is unsigned, so the condition
is always false.

Signed-off-by: Jiri Bohac 
Reported-by: Julia Lawall 
Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index cc7c26d..7a043a4 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1248,7 +1248,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, 
struct inet6_ifaddr *i
tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
idev->desync_factor;
/* guard against underflow in case of concurrent updates to cnf */
-   if (unlikely(tmp_prefered_lft < 0))
+   if (unlikely((long)tmp_prefered_lft < 0))
tmp_prefered_lft = 0;
tmp_prefered_lft = min_t(__u32, ifp->prefered_lft, tmp_prefered_lft);
tmp_plen = ifp->prefix_len;