UAF in ip6_do_table on 4.19 kernel

UAF in ip6_do_table on 4.19 kernel

[stranche]

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));
		}
	}

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);
	}

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?

Thanks,
Sean

---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

Re: UAF in ip6_do_table on 4.19 kernel

[Eric Dumazet]

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