Re: net/ipv4: use-after-free in ipv4_mtu

[Cong Wang]

On Thu, Apr 6, 2017 at 3:49 AM, Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:
> On Wed, 2017-04-05 at 15:33 -0700, Cong Wang wrote:
>
>> Good find! I missed the refcnt in rt_set_nexthop() before that commit.
>>
>> We need to revert that commit to restore the refcnt for fib_info.
>
> Well, there are other spots , in decnet and IPv6.

IPv6 is very different, it copies or steals the metrics from mx6_config:

static int fib6_commit_metrics(struct dst_entry *dst, struct mx6_config *mxc)
{
        if (!mxc->mx)
                return 0;

        if (dst->flags & DST_HOST) {
                u32 *mp = dst_metrics_write_ptr(dst);

                if (unlikely(!mp))
                        return -ENOMEM;

                fib6_copy_metrics(mp, mxc);
        } else {
                dst_init_metrics(dst, mxc->mx, false);

                /* We've stolen mx now. */
                mxc->mx = NULL;
        }

        return 0;
}

so probably doesn't need a refcnt.

Decnet has already done the refcnt'ing, see dn_fib_semantic_match().


>
> This is why my original mail stated the problem was in the calls to :
>
> dst_init_metrics(&rt->dst, fi->fib_metrics, true);
>
> Lets do not think in "reverting" spirit, but adding the missing bits.
>
> The problem here is that the metrics should not be freed until last user
> is gone.
>
> So maybe a refcount should be added to metrics, and we do not have to
> add a fib pointer again in all dsts.
>

Good point, but it is harder than just revert given that fact that dst metrics
is a magic pointer to an array and COW.