Re: UAF in ip6_do_table on 4.19 kernel

[Eric Dumazet <eric.dumazet@gmail.com>]

On 11/11/19 12:49 PM, stranche@codeaurora.org wrote:
> Hi all,
> 
> We recently had a crash reported to us on the 4.19 kernel where ip6_do_table() appeared to be referencing a jumpstack that had already been freed.
> Based on the dump, it appears that the scenario was a concurrent use of iptables-restore and active data transfer. The kernel has Florian's commit
> to wait in xt_replace_table instead of get_counters(), 80055dab5de0 ("netfilter: x_tables: make xt_replace_table wait until old rules are not used
> anymore"), so it appears that xt_replace_table is somehow returning prematurely, allowing __do_replace() to free the table while it is still in use.
> 
> After reviewing the code, we had a question about the following section:
>     /* ... so wait for even xt_recseq on all cpus */
>     for_each_possible_cpu(cpu) {
>         seqcount_t *s = &per_cpu(xt_recseq, cpu);
>         u32 seq = raw_read_seqcount(s);
> 
>         if (seq & 1) {
>             do {
>                 cond_resched();
>                 cpu_relax();
>             } while (seq == raw_read_seqcount(s));
>         }
>     }

The intent of this code is to check that each cpu went through a phase where the seq was even at least once.

> 
> Based on the other uses of seqcount locks, there should be a paired read_seqcount_retry() to mark the end of the read section like below:
>     for_each_possible_cpu(cpu) {
>         seqcount_t *s = &per_cpu(xt_recseq, cpu);
>         u32 seq;
> 
>         do {
>             seq = raw_read_seqcount(s);
>             if (seq & 1) {
>                 cond_resched();
>                 cpu_relax();
>             }
>         } while (read_seqcount_retry(s, seq);

This would loop possibly more times, since you exit if the count is _currently_ even.
 
If we are unlucky this could loop for a very long time.

>     }
> 
> These two snippets are very similar, as the original seems like it attempted to open-code this retry() helper, but there is a slight difference in
> the smp_rmb() placement relative to the "retry" read of the sequence value.
> Original:
>     READ_ONCE(s->sequence);
>     smp_rmb();
>     ... //check and resched
>     READ_ONCE(s->sequence);
>     smp_rmb();
>     ... //compare the two sequence values
> 
> Modified using read_seqcount_retry():
>     READ_ONCE(s->sequence);
>     smp_rmb();
>     ... //check and and resched
>     smp_rmb();
>     READ_ONCE(s->sequence);
>     ... //compare the two sequence values
> 
> Is it possible that this difference in ordering could lead to an incorrect read of the sequence in certain neurotic scenarios? Alternatively, is there
> some other place that this jumpstack could be freed while still in use?
> 

4.19 has many bugs really. Please upgrade to v4.19.83