Jan Engelhardt wrote:
> On Friday 2010-02-19 18:48, Tim Gardner wrote:
>> Consider the case when ip_pkt_list_tot==1; the first packet received is stored
>> in e->stamps[0] and e->index is initialized to 1. The next received packet
>> timestamp is then stored at e->stamps[1] in recent_entry_update(),
>> a buffer overflow because the maximum e->stamps[] index is 0.
>
>> @@ -173,10 +173,10 @@ recent_entry_init(struct recent_table *t, const union nf_inet_addr *addr,
>>
>> static void recent_entry_update(struct recent_table *t, struct recent_entry *e)
>> {
>> + e->index %= ip_pkt_list_tot;
>> e->stamps[e->index++] = jiffies;
>> if (e->index > e->nstamps)
>> e->nstamps = e->index;
>> - e->index %= ip_pkt_list_tot;
>> list_move_tail(&e->lru_list, &t->lru_list);
>> }
>
> Let's analyze in 3-step manner:
>
> Claim: writes always happen to e->stamps[0]
> Prereqs: ip_pkt_list_tot==1
> Proof:
> Start with assumption that e->index's possible values at the
> start of the function are {0}.
This assumption is the root of the bug. e->index is initialized to 1 in
recent_entry_init() which means that its already out of bounds when next
recent_entry_update() is called.
> The timestamp is thus always stored in e->stamps[0].
> e->index is bumped from 0 to 1.
> The %= op clamps it back to 0.
> The possible values at the end of the function are thus {0}.
> Assumption holds and matches the result set exactly.
> Outside of the function you will thus never see e->index != 0.
>
> This does not seem much different from your proposed patch,
> which reads like:
>
> Claim: same
> Prereq: same
> Proof:
> e->index's possible start values are {0,1}.
> The %= op clamps this to {0}.
> The timestamp is always stored in e->stamps[0].
> e->index is increased by one.
> The possible values at the end of the function are {1}.
> Assumption holds, but is a superset of the result set.
> Outside of the function, you may see e->index != 0.
>
>
> So both variations of the code do the same, except yours seems to
> have the additional potential pitfall that e->index is not within the
> ring of modulus after the function has been executed.
>
>
> Where would the thinko be?
>
rtg
--
Tim Gardner timg@xxxxxxx www.tpi.com
OR 503-601-0234 x102 MT 406-443-5357
--
To unsubscribe from this list: send the line "unsubscribe netfilter" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
