On Tue, Nov 03, 2015 at 21:40 +0100, Alexandr Nedvedicky wrote:
> Hello,
>
> Patch fixes potential use-after-free in pf_table.c:pfr_set_addrs():
>
> 463 for (i = 0; i < size; i++) {
> ...
> 483 q = pfr_lookup_addr(tmpkt, &ad, 1);
> 484 if (q != NULL) {
> 485 ad.pfra_fback = PFR_FB_DUPLICATE;
> 486 goto _skip;
> 487 }
> 488 p = pfr_create_kentry(&ad);
> 489 if (p == NULL)
> 490 senderr(ENOMEM);
> 491 if (pfr_route_kentry(tmpkt, p)) {
> 492 pfr_destroy_kentry(p);
> 493 ad.pfra_fback = PFR_FB_NONE;
> 494 } else {
> 495 SLIST_INSERT_HEAD(&addq, p, pfrke_workq);
> 496 ad.pfra_fback = PFR_FB_ADDED;
> 497 xadd++;
> 498 }
> 499 if (p->pfrke_type == PFRKE_COST)
> 500 kt->pfrkt_refcntcost++;
> 501 pfr_ktable_winfo_update(kt, p);
> 502 }
> 503 _skip:
> 504 if (flags & PFR_FLAG_FEEDBACK)
> 505 if (COPYOUT(&ad, addr+i, sizeof(ad), flags))
> 506 senderr(EFAULT);
> 507 }
>
>
> the things start to go downhill if PF takes branch at 492, where `p` gets
> freed. The code jumps to 499 & 501, where PF steps to dead pointer. I think
> the
> right thing is to use goto _skip; in that branch to avoid 499 et. al.
> completely.
>
I agree.
> path is below.
>
> OK?
>
OK mikeb
> thanks and
> regards
> sasha
>
> --------8<---------------8<---------------8<------------------8<--------
>
> Index: pf_table.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_table.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 pf_table.c
> --- pf_table.c 7 Oct 2015 11:57:44 -0000 1.115
> +++ pf_table.c 3 Nov 2015 20:15:26 -0000
> @@ -491,6 +491,7 @@ pfr_set_addrs(struct pfr_table *tbl, str
> if (pfr_route_kentry(tmpkt, p)) {
> pfr_destroy_kentry(p);
> ad.pfra_fback = PFR_FB_NONE;
> + goto _skip;
> } else {
> SLIST_INSERT_HEAD(&addq, p, pfrke_workq);
> ad.pfra_fback = PFR_FB_ADDED;
>
