| Message ID | 20161018150154.4tpivtvxygp7kjpd@dwarf.suse.cz |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jiri Bohac <jbohac@suse.cz> 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 <jbohac@suse.cz> > Reported-by: Julia Lawall <julia.lawall@lip6.fr> > Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR") Does the check make any sense at all? I'd say just remove it.
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,
From: Jiri Bohac <jbohac@suse.cz> 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.
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;
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 <jbohac@suse.cz> Reported-by: Julia Lawall <julia.lawall@lip6.fr> Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")