Re: Possible null deref on pf.c
On Fri, Feb 12, 2021 at 01:20:01PM +0100, Alexander Bluhm wrote:
> On Fri, Feb 12, 2021 at 01:11:24PM +0100, Claudio Jeker wrote:
> > On Fri, Feb 12, 2021 at 12:03:49PM +, Ricardo Mestre wrote:
> > > This was reported on CID 1501718, ifp starts as NULL and then might be
> > > deref'ed.
>
>
> > This code is strange, the scope for the IPv6 address needs to be pulled
> > out of s (pf_state) somehow. Also is the state using embedded or
> > not-embedded scope addresses?
>
> I was already discussung the issue with dlg@
>
> We both think that the code is not necessary. The address comes
> from pf configuration. pf does nor work correctly with IPv6
> link-local anyway. I think the only way to fix pf with link-local,
> is to embed the scope for all addresses within pf.
>
> Current code is broken, embeding here cannot work, pf link-local
> needs rework, remove code makes rework easier.
>
> ok?
I came to the same conclusion. I agree that this is currently unfixable
and so removing this bit of code is correct.
OK claudio@
> bluhm
>
> Index: net/pf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.1108
> diff -u -p -r1.1108 pf.c
> --- net/pf.c 4 Feb 2021 00:55:41 - 1.1108
> +++ net/pf.c 12 Feb 2021 12:06:47 -
> @@ -6156,8 +6156,6 @@ pf_route6(struct pf_pdesc *pd, struct pf
> dst->sin6_addr = s->rt_addr.v6;
> rtableid = m0->m_pkthdr.ph_rtableid;
>
> - if (IN6_IS_SCOPE_EMBED(>sin6_addr))
> - dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
> rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
> if (!rtisvalid(rt)) {
> if (s->rt != PF_DUPTO) {
>
--
:wq Claudio
Re: Possible null deref on pf.c
On Fri, Feb 12, 2021 at 01:11:24PM +0100, Claudio Jeker wrote:
> On Fri, Feb 12, 2021 at 12:03:49PM +, Ricardo Mestre wrote:
> > This was reported on CID 1501718, ifp starts as NULL and then might be
> > deref'ed.
> This code is strange, the scope for the IPv6 address needs to be pulled
> out of s (pf_state) somehow. Also is the state using embedded or
> not-embedded scope addresses?
I was already discussung the issue with dlg@
We both think that the code is not necessary. The address comes
from pf configuration. pf does nor work correctly with IPv6
link-local anyway. I think the only way to fix pf with link-local,
is to embed the scope for all addresses within pf.
Current code is broken, embeding here cannot work, pf link-local
needs rework, remove code makes rework easier.
ok?
bluhm
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1108
diff -u -p -r1.1108 pf.c
--- net/pf.c4 Feb 2021 00:55:41 - 1.1108
+++ net/pf.c12 Feb 2021 12:06:47 -
@@ -6156,8 +6156,6 @@ pf_route6(struct pf_pdesc *pd, struct pf
dst->sin6_addr = s->rt_addr.v6;
rtableid = m0->m_pkthdr.ph_rtableid;
- if (IN6_IS_SCOPE_EMBED(>sin6_addr))
- dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
if (!rtisvalid(rt)) {
if (s->rt != PF_DUPTO) {
Re: Possible null deref on pf.c
On Fri, Feb 12, 2021 at 12:03:49PM +, Ricardo Mestre wrote: > Hi, > > This was reported on CID 1501718, ifp starts as NULL and then might be > deref'ed. > > The question is does the below make any sense to solve it since I don't know > what I'm doing? :) > > What do you net gurus say? > > Index: pf.c > === > RCS file: /cvs/src/sys/net/pf.c,v > retrieving revision 1.1108 > diff -u -p -u -r1.1108 pf.c > --- pf.c 4 Feb 2021 00:55:41 - 1.1108 > +++ pf.c 12 Feb 2021 11:52:31 - > @@ -6156,6 +6156,10 @@ pf_route6(struct pf_pdesc *pd, struct pf > dst->sin6_addr = s->rt_addr.v6; > rtableid = m0->m_pkthdr.ph_rtableid; > > + ifp = if_get(rt->rt_ifidx); > + if (ifp == NULL) > + goto bad; > + > if (IN6_IS_SCOPE_EMBED(>sin6_addr)) > dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index); > rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid); > @@ -6168,10 +6172,6 @@ pf_route6(struct pf_pdesc *pd, struct pf > ip6stat_inc(ip6s_noroute); > goto bad; > } > - > - ifp = if_get(rt->rt_ifidx); > - if (ifp == NULL) > - goto bad; > > /* A locally generated packet may have invalid source address. */ > if (IN6_IS_ADDR_LOOPBACK(>ip6_src) && This can't be right. rt is only valid after rtalloc() which is now called afterwards. This code is strange, the scope for the IPv6 address needs to be pulled out of s (pf_state) somehow. Also is the state using embedded or not-embedded scope addresses? Last but not least, I hate IPv6 and their scoped addressing. Worst feature ever. -- :wq Claudio
Possible null deref on pf.c
Hi, This was reported on CID 1501718, ifp starts as NULL and then might be deref'ed. The question is does the below make any sense to solve it since I don't know what I'm doing? :) What do you net gurus say? Index: pf.c === RCS file: /cvs/src/sys/net/pf.c,v retrieving revision 1.1108 diff -u -p -u -r1.1108 pf.c --- pf.c4 Feb 2021 00:55:41 - 1.1108 +++ pf.c12 Feb 2021 11:52:31 - @@ -6156,6 +6156,10 @@ pf_route6(struct pf_pdesc *pd, struct pf dst->sin6_addr = s->rt_addr.v6; rtableid = m0->m_pkthdr.ph_rtableid; + ifp = if_get(rt->rt_ifidx); + if (ifp == NULL) + goto bad; + if (IN6_IS_SCOPE_EMBED(>sin6_addr)) dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index); rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid); @@ -6168,10 +6172,6 @@ pf_route6(struct pf_pdesc *pd, struct pf ip6stat_inc(ip6s_noroute); goto bad; } - - ifp = if_get(rt->rt_ifidx); - if (ifp == NULL) - goto bad; /* A locally generated packet may have invalid source address. */ if (IN6_IS_ADDR_LOOPBACK(>ip6_src) &&
