On Wed, Jun 29, 2022 at 12:42:13AM +0200, Alexandr Nedvedicky wrote:
> for example here in current ip6_input_if() we may leak memory
> on error:
Curently we do not leak memory. We free the mbuf in the callee,
and check the return code in the caller.
> another option would be to revisit all places where we call ip6_hbhchcheck()
> and adjust caller accordingly.
I added an comment that described the error handling before commit.
> In my opinion all places where we
> making ip6_hbhchcheck() to also free packet on error should be the
> right thing to do.
Next diff passes the pointer further down and m_freemp(mp) sets it
to NULL. Also sort the parameters so that the mbuf is always first.
Note that icmp6_error() frees the mbuf, but does not reset the
pointer. Fixing that would touch much more code.
ok?
bluhm
Index: netinet6/dest6.c
===================================================================
RCS file: /cvs/src/sys/netinet6/dest6.c,v
retrieving revision 1.18
diff -u -p -r1.18 dest6.c
--- netinet6/dest6.c 22 Feb 2022 01:15:02 -0000 1.18
+++ netinet6/dest6.c 29 Jun 2022 12:10:38 -0000
@@ -49,18 +49,17 @@
int
dest6_input(struct mbuf **mp, int *offp, int proto, int af)
{
- struct mbuf *m = *mp;
int off = *offp, dstoptlen, optlen;
struct ip6_dest *dstopts;
u_int8_t *opt;
/* validation of the length of the header */
- IP6_EXTHDR_GET(dstopts, struct ip6_dest *, m, off, sizeof(*dstopts));
+ IP6_EXTHDR_GET(dstopts, struct ip6_dest *, *mp, off, sizeof(*dstopts));
if (dstopts == NULL)
return IPPROTO_DONE;
dstoptlen = (dstopts->ip6d_len + 1) << 3;
- IP6_EXTHDR_GET(dstopts, struct ip6_dest *, m, off, dstoptlen);
+ IP6_EXTHDR_GET(dstopts, struct ip6_dest *, *mp, off, dstoptlen);
if (dstopts == NULL)
return IPPROTO_DONE;
off += dstoptlen;
@@ -83,8 +82,8 @@ dest6_input(struct mbuf **mp, int *offp,
optlen = *(opt + 1) + 2;
break;
default: /* unknown option */
- optlen = ip6_unknown_opt(opt, m,
- opt - mtod(m, u_int8_t *));
+ optlen = ip6_unknown_opt(mp, opt,
+ opt - mtod(*mp, u_int8_t *));
if (optlen == -1)
return (IPPROTO_DONE);
optlen += 2;
@@ -96,6 +95,6 @@ dest6_input(struct mbuf **mp, int *offp,
return (dstopts->ip6d_nxt);
bad:
- m_freem(m);
+ m_freemp(mp);
return (IPPROTO_DONE);
}
Index: netinet6/ip6_input.c
===================================================================
RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.247
diff -u -p -r1.247 ip6_input.c
--- netinet6/ip6_input.c 29 Jun 2022 11:22:10 -0000 1.247
+++ netinet6/ip6_input.c 29 Jun 2022 12:10:38 -0000
@@ -123,7 +123,7 @@ int ip6_ours(struct mbuf **, int *, int,
int ip6_local(struct mbuf **, int *, int, int);
int ip6_check_rh0hdr(struct mbuf *, int *);
int ip6_hbhchcheck(struct mbuf **, int *, int *);
-int ip6_hopopts_input(u_int32_t *, u_int32_t *, struct mbuf **, int *);
+int ip6_hopopts_input(struct mbuf **, int *, u_int32_t *, u_int32_t *);
struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
int ip6_sysctl_soiikey(void *, size_t *, void *, size_t);
@@ -616,7 +616,7 @@ ip6_hbhchcheck(struct mbuf **mp, int *of
if (ip6->ip6_nxt == IPPROTO_HOPOPTS) {
struct ip6_hbh *hbh;
- if (ip6_hopopts_input(&plen, &rtalert, mp, offp))
+ if (ip6_hopopts_input(mp, offp, &plen, &rtalert))
goto bad; /* m have already been freed */
/* adjust pointer */
@@ -758,8 +758,8 @@ ip6_check_rh0hdr(struct mbuf *m, int *of
* rtalertp - XXX: should be stored in a more smart way
*/
int
-ip6_hopopts_input(u_int32_t *plenp, u_int32_t *rtalertp, struct mbuf **mp,
- int *offp)
+ip6_hopopts_input(struct mbuf **mp, int *offp, u_int32_t *plenp,
+ u_int32_t *rtalertp)
{
int off = *offp, hbhlen;
struct ip6_hbh *hbh;
@@ -781,7 +781,7 @@ ip6_hopopts_input(u_int32_t *plenp, u_in
off += hbhlen;
hbhlen -= sizeof(struct ip6_hbh);
- if (ip6_process_hopopts(*mp, (u_int8_t *)hbh + sizeof(struct ip6_hbh),
+ if (ip6_process_hopopts(mp, (u_int8_t *)hbh + sizeof(struct ip6_hbh),
hbhlen, rtalertp, plenp) < 0)
return (-1);
@@ -794,13 +794,14 @@ ip6_hopopts_input(u_int32_t *plenp, u_in
* This function is separate from ip6_hopopts_input() in order to
* handle a case where the sending node itself process its hop-by-hop
* options header. In such a case, the function is called from ip6_output().
+ * On error free mbuf and return -1.
*
* The function assumes that hbh header is located right after the IPv6 header
* (RFC2460 p7), opthead is pointer into data content in m, and opthead to
* opthead + hbhlen is located in continuous memory region.
*/
int
-ip6_process_hopopts(struct mbuf *m, u_int8_t *opthead, int hbhlen,
+ip6_process_hopopts(struct mbuf **mp, u_int8_t *opthead, int hbhlen,
u_int32_t *rtalertp, u_int32_t *plenp)
{
struct ip6_hdr *ip6;
@@ -830,7 +831,7 @@ ip6_process_hopopts(struct mbuf *m, u_in
}
if (*(opt + 1) != IP6OPT_RTALERT_LEN - 2) {
/* XXX stat */
- icmp6_error(m, ICMP6_PARAM_PROB,
+ icmp6_error(*mp, ICMP6_PARAM_PROB,
ICMP6_PARAMPROB_HEADER,
erroff + opt + 1 - opthead);
return (-1);
@@ -847,7 +848,7 @@ ip6_process_hopopts(struct mbuf *m, u_in
}
if (*(opt + 1) != IP6OPT_JUMBO_LEN - 2) {
/* XXX stat */
- icmp6_error(m, ICMP6_PARAM_PROB,
+ icmp6_error(*mp, ICMP6_PARAM_PROB,
ICMP6_PARAMPROB_HEADER,
erroff + opt + 1 - opthead);
return (-1);
@@ -858,10 +859,10 @@ ip6_process_hopopts(struct mbuf *m, u_in
* IPv6 packets that have non 0 payload length
* must not contain a jumbo payload option.
*/
- ip6 = mtod(m, struct ip6_hdr *);
+ ip6 = mtod(*mp, struct ip6_hdr *);
if (ip6->ip6_plen) {
ip6stat_inc(ip6s_badoptions);
- icmp6_error(m, ICMP6_PARAM_PROB,
+ icmp6_error(*mp, ICMP6_PARAM_PROB,
ICMP6_PARAMPROB_HEADER,
erroff + opt - opthead);
return (-1);
@@ -885,7 +886,7 @@ ip6_process_hopopts(struct mbuf *m, u_in
*/
if (*plenp != 0) {
ip6stat_inc(ip6s_badoptions);
- icmp6_error(m, ICMP6_PARAM_PROB,
+ icmp6_error(*mp, ICMP6_PARAM_PROB,
ICMP6_PARAMPROB_HEADER,
erroff + opt + 2 - opthead);
return (-1);
@@ -897,7 +898,7 @@ ip6_process_hopopts(struct mbuf *m, u_in
*/
if (jumboplen <= IPV6_MAXPACKET) {
ip6stat_inc(ip6s_badoptions);
- icmp6_error(m, ICMP6_PARAM_PROB,
+ icmp6_error(*mp, ICMP6_PARAM_PROB,
ICMP6_PARAMPROB_HEADER,
erroff + opt + 2 - opthead);
return (-1);
@@ -910,7 +911,7 @@ ip6_process_hopopts(struct mbuf *m, u_in
ip6stat_inc(ip6s_toosmall);
goto bad;
}
- optlen = ip6_unknown_opt(opt, m,
+ optlen = ip6_unknown_opt(mp, opt,
erroff + opt - opthead);
if (optlen == -1)
return (-1);
@@ -922,7 +923,7 @@ ip6_process_hopopts(struct mbuf *m, u_in
return (0);
bad:
- m_freem(m);
+ m_freemp(mp);
return (-1);
}
@@ -931,9 +932,10 @@ ip6_process_hopopts(struct mbuf *m, u_in
* The third argument `off' is the offset from the IPv6 header to the option,
* which allows returning an ICMPv6 error even if the IPv6 header and the
* option header are not continuous.
+ * On error free mbuf and return -1.
*/
int
-ip6_unknown_opt(u_int8_t *optp, struct mbuf *m, int off)
+ip6_unknown_opt(struct mbuf **mp, u_int8_t *optp, int off)
{
struct ip6_hdr *ip6;
@@ -941,25 +943,25 @@ ip6_unknown_opt(u_int8_t *optp, struct m
case IP6OPT_TYPE_SKIP: /* ignore the option */
return ((int)*(optp + 1));
case IP6OPT_TYPE_DISCARD: /* silently discard */
- m_freem(m);
+ m_freemp(mp);
return (-1);
case IP6OPT_TYPE_FORCEICMP: /* send ICMP even if multicasted */
ip6stat_inc(ip6s_badoptions);
- icmp6_error(m, ICMP6_PARAM_PROB, ICMP6_PARAMPROB_OPTION, off);
+ icmp6_error(*mp, ICMP6_PARAM_PROB, ICMP6_PARAMPROB_OPTION, off);
return (-1);
case IP6OPT_TYPE_ICMP: /* send ICMP if not multicasted */
ip6stat_inc(ip6s_badoptions);
- ip6 = mtod(m, struct ip6_hdr *);
+ ip6 = mtod(*mp, struct ip6_hdr *);
if (IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst) ||
- (m->m_flags & (M_BCAST|M_MCAST)))
- m_freem(m);
+ ((*mp)->m_flags & (M_BCAST|M_MCAST)))
+ m_freemp(mp);
else
- icmp6_error(m, ICMP6_PARAM_PROB,
+ icmp6_error(*mp, ICMP6_PARAM_PROB,
ICMP6_PARAMPROB_OPTION, off);
return (-1);
}
- m_freem(m); /* XXX: NOTREACHED */
+ m_freemp(mp); /* XXX: NOTREACHED */
return (-1);
}
Index: netinet6/ip6_output.c
===================================================================
RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
retrieving revision 1.268
diff -u -p -r1.268 ip6_output.c
--- netinet6/ip6_output.c 22 Feb 2022 01:35:41 -0000 1.268
+++ netinet6/ip6_output.c 29 Jun 2022 12:10:38 -0000
@@ -619,7 +619,7 @@ reroute:
u_int32_t plen = 0; /* no more than 1 jumbo payload option! */
m->m_pkthdr.ph_ifidx = ifp->if_index;
- if (ip6_process_hopopts(m, (u_int8_t *)(hbh + 1),
+ if (ip6_process_hopopts(&m, (u_int8_t *)(hbh + 1),
((hbh->ip6h_len + 1) << 3) - sizeof(struct ip6_hbh),
&rtalert, &plen) < 0) {
/* m was already freed at this point */
Index: netinet6/ip6_var.h
===================================================================
RCS file: /cvs/src/sys/netinet6/ip6_var.h,v
retrieving revision 1.92
diff -u -p -r1.92 ip6_var.h
--- netinet6/ip6_var.h 5 May 2022 13:57:41 -0000 1.92
+++ netinet6/ip6_var.h 29 Jun 2022 12:10:38 -0000
@@ -307,12 +307,12 @@ void ip6intr(void);
int ip6_input_if(struct mbuf **, int *, int, int, struct ifnet *);
void ip6_freepcbopts(struct ip6_pktopts *);
void ip6_freemoptions(struct ip6_moptions *);
-int ip6_unknown_opt(u_int8_t *, struct mbuf *, int);
+int ip6_unknown_opt(struct mbuf **, u_int8_t *, int);
int ip6_get_prevhdr(struct mbuf *, int);
int ip6_nexthdr(struct mbuf *, int, int, int *);
int ip6_lasthdr(struct mbuf *, int, int, int *);
int ip6_mforward(struct ip6_hdr *, struct ifnet *, struct mbuf *);
-int ip6_process_hopopts(struct mbuf *, u_int8_t *, int, u_int32_t *,
+int ip6_process_hopopts(struct mbuf **, u_int8_t *, int, u_int32_t *,
u_int32_t *);
void ip6_savecontrol(struct inpcb *, struct mbuf *, struct mbuf **);
int ip6_sysctl(int *, u_int, void *, size_t *, void *, size_t);
