Hi,

pfkeyv2.c:1091:pfkeyv2_send(struct socket *so, void *message, int len)
leaks memory in the SADB_REGISTER case (line 1579).
It reuses void *freeme multiple times to build up void *headers[].

headers[] are bcopy'ed to another buffer inside of pfkeyv2_sendmessage()
(line 2064) so as the name implies *freeme is safe to be freed at the end.

Found by llvm/scan-build.
Also don't check for NULL before free(), could do a pass over the entire file.

Only compile tested.

- Ben 

Index: pfkeyv2.c
===================================================================
RCS file: /cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.198
diff -u -p -r1.198 pfkeyv2.c
--- pfkeyv2.c   17 Jul 2019 18:52:46 -0000      1.198
+++ pfkeyv2.c   3 Mar 2020 17:58:22 -0000
@@ -1098,7 +1098,8 @@ pfkeyv2_send(struct socket *so, void *me
        struct radix_node_head *rnh;
        struct radix_node *rn = NULL;
        struct pkpcb *kp, *bkp;
-       void *freeme = NULL, *bckptr = NULL;
+       void *freeme = NULL, *freeme2 = NULL, *freeme3 = NULL;
+       void *bckptr = NULL;
        void *headers[SADB_EXT_MAX + 1];
        union sockaddr_union *sunionp;
        struct tdb *sa1 = NULL, *sa2 = NULL;
@@ -1605,7 +1606,7 @@ pfkeyv2_send(struct socket *so, void *me
 
                i = sizeof(struct sadb_supported) + sizeof(aalgs);
 
-               if (!(freeme = malloc(i, M_PFKEY, M_NOWAIT | M_ZERO))) {
+               if (!(freeme2 = malloc(i, M_PFKEY, M_NOWAIT | M_ZERO))) {
                        rval = ENOMEM;
                        goto ret;
                }
@@ -1616,34 +1617,34 @@ pfkeyv2_send(struct socket *so, void *me
                    (1 << ((struct sadb_msg *)message)->sadb_msg_satype);
                keyunlock(kp, s);
 
-               ssup = (struct sadb_supported *) freeme;
+               ssup = (struct sadb_supported *) freeme2;
                ssup->sadb_supported_len = i / sizeof(uint64_t);
 
                {
-                       void *p = freeme + sizeof(struct sadb_supported);
+                       void *p = freeme2 + sizeof(struct sadb_supported);
 
                        bcopy(&aalgs[0], p, sizeof(aalgs));
                }
 
-               headers[SADB_EXT_SUPPORTED_AUTH] = freeme;
+               headers[SADB_EXT_SUPPORTED_AUTH] = freeme2;
 
                i = sizeof(struct sadb_supported) + sizeof(calgs);
 
-               if (!(freeme = malloc(i, M_PFKEY, M_NOWAIT | M_ZERO))) {
+               if (!(freeme3 = malloc(i, M_PFKEY, M_NOWAIT | M_ZERO))) {
                        rval = ENOMEM;
                        goto ret;
                }
 
-               ssup = (struct sadb_supported *) freeme;
+               ssup = (struct sadb_supported *) freeme3;
                ssup->sadb_supported_len = i / sizeof(uint64_t);
 
                {
-                       void *p = freeme + sizeof(struct sadb_supported);
+                       void *p = freeme3 + sizeof(struct sadb_supported);
 
                        bcopy(&calgs[0], p, sizeof(calgs));
                }
 
-               headers[SADB_X_EXT_SUPPORTED_COMP] = freeme;
+               headers[SADB_X_EXT_SUPPORTED_COMP] = freeme3;
 
                break;
 
@@ -2064,14 +2065,14 @@ ret:
 
 realret:
 
-       if (freeme)
-               free(freeme, M_PFKEY, 0);
+       free(freeme, M_PFKEY, 0);
+       free(freeme2, M_PFKEY, 0);
+       free(freeme3, M_PFKEY, 0);
 
        explicit_bzero(message, len);
        free(message, M_PFKEY, 0);
 
-       if (sa1)
-               free(sa1, M_PFKEY, 0);
+       free(sa1, M_PFKEY, 0);
 
        return (rval);
 }

Reply via email to