Re: ip_icmp reference fix
On Mon, Aug 22, 2016 at 01:21:47PM +0200, Martin Pieuchot wrote:
> When it comes to reference counting in the receiving path, route entries
> act as proxy for interface addresses. In other words you CANNOT
> dereference ``rt->rt_ifa'' after calling rtfree(9).
>
> Diff below fixes that in icmp_reflect(), ok?
Good catch.
OK claudio@
> Index: netmpls/mpls_input.c
> ===
> RCS file: /cvs/src/sys/netmpls/mpls_input.c,v
> retrieving revision 1.56
> diff -u -p -r1.56 mpls_input.c
> --- netmpls/mpls_input.c 11 Jul 2016 09:23:06 - 1.56
> +++ netmpls/mpls_input.c 22 Aug 2016 11:20:48 -
> @@ -385,8 +385,9 @@ mpls_do_error(struct mbuf *m, int type,
> m_freem(m);
> return (NULL);
> }
> - rtfree(rt);
> + /* It is safe to dereference ``ia'' iff ``rt'' is valid. */
> error = icmp_reflect(m, NULL, ia);
> + rtfree(rt);
> if (error)
> return (NULL);
>
> Index: netinet/ip_icmp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.151
> diff -u -p -r1.151 ip_icmp.c
> --- netinet/ip_icmp.c 9 Dec 2015 09:27:40 - 1.151
> +++ netinet/ip_icmp.c 22 Aug 2016 11:21:02 -
> @@ -702,7 +702,7 @@ icmp_reflect(struct mbuf *m, struct mbuf
> struct ip *ip = mtod(m, struct ip *);
> struct mbuf *opts = NULL;
> struct sockaddr_in sin;
> - struct rtentry *rt;
> + struct rtentry *rt = NULL;
> int optlen = (ip->ip_hl << 2) - sizeof(struct ip);
> u_int rtableid;
>
> @@ -733,7 +733,6 @@ icmp_reflect(struct mbuf *m, struct mbuf
> if (rtisvalid(rt) &&
> ISSET(rt->rt_flags, RTF_LOCAL|RTF_BROADCAST))
> ia = ifatoia(rt->rt_ifa);
> - rtfree(rt);
> }
>
> /*
> @@ -742,6 +741,8 @@ icmp_reflect(struct mbuf *m, struct mbuf
>* drop the packet as there is no path to the host.
>*/
> if (ia == NULL) {
> + rtfree(rt);
> +
> memset(&sin, 0, sizeof(sin));
> sin.sin_len = sizeof(sin);
> sin.sin_family = AF_INET;
> @@ -756,12 +757,14 @@ icmp_reflect(struct mbuf *m, struct mbuf
> }
>
> ia = ifatoia(rt->rt_ifa);
> - rtfree(rt);
> }
>
> ip->ip_dst = ip->ip_src;
> - ip->ip_src = ia->ia_addr.sin_addr;
> ip->ip_ttl = MAXTTL;
> +
> + /* It is safe to dereference ``ia'' iff ``rt'' is valid. */
> + ip->ip_src = ia->ia_addr.sin_addr;
> + rtfree(rt);
>
> if (optlen > 0) {
> u_char *cp;
>
--
:wq Claudio
Re: ip_icmp reference fix
On Mon, Aug 22, 2016 at 01:21:47PM +0200, Martin Pieuchot wrote:
> When it comes to reference counting in the receiving path, route entries
> act as proxy for interface addresses. In other words you CANNOT
> dereference ``rt->rt_ifa'' after calling rtfree(9).
>
> Diff below fixes that in icmp_reflect(), ok?
OK bluhm@
>
> Index: netmpls/mpls_input.c
> ===
> RCS file: /cvs/src/sys/netmpls/mpls_input.c,v
> retrieving revision 1.56
> diff -u -p -r1.56 mpls_input.c
> --- netmpls/mpls_input.c 11 Jul 2016 09:23:06 - 1.56
> +++ netmpls/mpls_input.c 22 Aug 2016 11:20:48 -
> @@ -385,8 +385,9 @@ mpls_do_error(struct mbuf *m, int type,
> m_freem(m);
> return (NULL);
> }
> - rtfree(rt);
> + /* It is safe to dereference ``ia'' iff ``rt'' is valid. */
> error = icmp_reflect(m, NULL, ia);
> + rtfree(rt);
> if (error)
> return (NULL);
>
> Index: netinet/ip_icmp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.151
> diff -u -p -r1.151 ip_icmp.c
> --- netinet/ip_icmp.c 9 Dec 2015 09:27:40 - 1.151
> +++ netinet/ip_icmp.c 22 Aug 2016 11:21:02 -
> @@ -702,7 +702,7 @@ icmp_reflect(struct mbuf *m, struct mbuf
> struct ip *ip = mtod(m, struct ip *);
> struct mbuf *opts = NULL;
> struct sockaddr_in sin;
> - struct rtentry *rt;
> + struct rtentry *rt = NULL;
> int optlen = (ip->ip_hl << 2) - sizeof(struct ip);
> u_int rtableid;
>
> @@ -733,7 +733,6 @@ icmp_reflect(struct mbuf *m, struct mbuf
> if (rtisvalid(rt) &&
> ISSET(rt->rt_flags, RTF_LOCAL|RTF_BROADCAST))
> ia = ifatoia(rt->rt_ifa);
> - rtfree(rt);
> }
>
> /*
> @@ -742,6 +741,8 @@ icmp_reflect(struct mbuf *m, struct mbuf
>* drop the packet as there is no path to the host.
>*/
> if (ia == NULL) {
> + rtfree(rt);
> +
> memset(&sin, 0, sizeof(sin));
> sin.sin_len = sizeof(sin);
> sin.sin_family = AF_INET;
> @@ -756,12 +757,14 @@ icmp_reflect(struct mbuf *m, struct mbuf
> }
>
> ia = ifatoia(rt->rt_ifa);
> - rtfree(rt);
> }
>
> ip->ip_dst = ip->ip_src;
> - ip->ip_src = ia->ia_addr.sin_addr;
> ip->ip_ttl = MAXTTL;
> +
> + /* It is safe to dereference ``ia'' iff ``rt'' is valid. */
> + ip->ip_src = ia->ia_addr.sin_addr;
> + rtfree(rt);
>
> if (optlen > 0) {
> u_char *cp;
Re: ip_icmp reference fix
Martin Pieuchot writes: > When it comes to reference counting in the receiving path, route entries > act as proxy for interface addresses. In other words you CANNOT > dereference ``rt->rt_ifa'' after calling rtfree(9). > > Diff below fixes that in icmp_reflect(), ok? ok jca@ -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
ip_icmp reference fix
When it comes to reference counting in the receiving path, route entries
act as proxy for interface addresses. In other words you CANNOT
dereference ``rt->rt_ifa'' after calling rtfree(9).
Diff below fixes that in icmp_reflect(), ok?
Index: netmpls/mpls_input.c
===
RCS file: /cvs/src/sys/netmpls/mpls_input.c,v
retrieving revision 1.56
diff -u -p -r1.56 mpls_input.c
--- netmpls/mpls_input.c11 Jul 2016 09:23:06 - 1.56
+++ netmpls/mpls_input.c22 Aug 2016 11:20:48 -
@@ -385,8 +385,9 @@ mpls_do_error(struct mbuf *m, int type,
m_freem(m);
return (NULL);
}
- rtfree(rt);
+ /* It is safe to dereference ``ia'' iff ``rt'' is valid. */
error = icmp_reflect(m, NULL, ia);
+ rtfree(rt);
if (error)
return (NULL);
Index: netinet/ip_icmp.c
===
RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.151
diff -u -p -r1.151 ip_icmp.c
--- netinet/ip_icmp.c 9 Dec 2015 09:27:40 - 1.151
+++ netinet/ip_icmp.c 22 Aug 2016 11:21:02 -
@@ -702,7 +702,7 @@ icmp_reflect(struct mbuf *m, struct mbuf
struct ip *ip = mtod(m, struct ip *);
struct mbuf *opts = NULL;
struct sockaddr_in sin;
- struct rtentry *rt;
+ struct rtentry *rt = NULL;
int optlen = (ip->ip_hl << 2) - sizeof(struct ip);
u_int rtableid;
@@ -733,7 +733,6 @@ icmp_reflect(struct mbuf *m, struct mbuf
if (rtisvalid(rt) &&
ISSET(rt->rt_flags, RTF_LOCAL|RTF_BROADCAST))
ia = ifatoia(rt->rt_ifa);
- rtfree(rt);
}
/*
@@ -742,6 +741,8 @@ icmp_reflect(struct mbuf *m, struct mbuf
* drop the packet as there is no path to the host.
*/
if (ia == NULL) {
+ rtfree(rt);
+
memset(&sin, 0, sizeof(sin));
sin.sin_len = sizeof(sin);
sin.sin_family = AF_INET;
@@ -756,12 +757,14 @@ icmp_reflect(struct mbuf *m, struct mbuf
}
ia = ifatoia(rt->rt_ifa);
- rtfree(rt);
}
ip->ip_dst = ip->ip_src;
- ip->ip_src = ia->ia_addr.sin_addr;
ip->ip_ttl = MAXTTL;
+
+ /* It is safe to dereference ``ia'' iff ``rt'' is valid. */
+ ip->ip_src = ia->ia_addr.sin_addr;
+ rtfree(rt);
if (optlen > 0) {
u_char *cp;
