netdev - Re: [PATCH net-next 1/9] net: sctp: sctp_ssnmap: remove 'malloced' element from struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20130417191854.GN5149@hmsreliant.think-freely.org>
Date:	Wed, 17 Apr 2013 15:18:54 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Daniel Borkmann <dborkman@...hat.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	linux-sctp@...r.kernel.org, Vlad Yasevich <vyasevich@...il.com>,
	David Laight <David.Laight@...LAB.COM>
Subject: Re: [PATCH net-next 1/9] net: sctp: sctp_ssnmap: remove 'malloced'
 element from struct

On Wed, Apr 17, 2013 at 07:17:18PM +0200, Daniel Borkmann wrote:
> On 04/17/2013 02:52 PM, Daniel Borkmann wrote:
> >On 04/17/2013 02:45 PM, Neil Horman wrote:
> >>On Tue, Apr 16, 2013 at 11:07:10PM +0200, Daniel Borkmann wrote:
> >>>sctp_ssnmap_init() can only be called from sctp_ssnmap_new()
> >>>where malloced is always set to 1. Thus, when we call
> >>>sctp_ssnmap_free() the test for map->malloced evaluates always
> >>>to true.
> >>>
> >>>Signed-off-by: Daniel Borkmann <dborkman@...hat.com>
> >>>---
> >>>  include/net/sctp/structs.h |  1 -
> >>>  net/sctp/ssnmap.c          | 23 ++++++++++++-----------
> >>>  2 files changed, 12 insertions(+), 12 deletions(-)
> >>>
> >>>diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> >>>index e12aa77..3c1bb8d 100644
> >>>--- a/include/net/sctp/structs.h
> >>>+++ b/include/net/sctp/structs.h
> >>>@@ -399,7 +399,6 @@ struct sctp_stream {
> >>>  struct sctp_ssnmap {
> >>>      struct sctp_stream in;
> >>>      struct sctp_stream out;
> >>>-    int malloced;
> >>>  };
> >>>
> >>>  struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
> >>>diff --git a/net/sctp/ssnmap.c b/net/sctp/ssnmap.c
> >>>index 825ea94..da86035 100644
> >>>--- a/net/sctp/ssnmap.c
> >>>+++ b/net/sctp/ssnmap.c
> >>>@@ -74,7 +74,6 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
> >>>      if (!sctp_ssnmap_init(retval, in, out))
> >>>          goto fail_map;
> >>>
> >>>-    retval->malloced = 1;
> >>>      SCTP_DBG_OBJCNT_INC(ssnmap);
> >>>
> >>>      return retval;
> >>>@@ -118,14 +117,16 @@ void sctp_ssnmap_clear(struct sctp_ssnmap *map)
> >>>  /* Dispose of a ssnmap.  */
> >>>  void sctp_ssnmap_free(struct sctp_ssnmap *map)
> >>>  {
> >>>-    if (map && map->malloced) {
> >>>-        int size;
> >>>-
> >>>-        size = sctp_ssnmap_size(map->in.len, map->out.len);
> >>>-        if (size <= KMALLOC_MAX_SIZE)
> >>>-            kfree(map);
> >>>-        else
> >>>-            free_pages((unsigned long)map, get_order(size));
> >>>-        SCTP_DBG_OBJCNT_DEC(ssnmap);
> >>>-    }
> >>>+    int size;
> >>>+
> >>>+    if (unlikely(!map))
> >>>+        return;
> >>>+
> >>>+    size = sctp_ssnmap_size(map->in.len, map->out.len);
> >>>+    if (size <= KMALLOC_MAX_SIZE)
> >>>+        kfree(map);
> >>>+    else
> >>>+        free_pages((unsigned long)map, get_order(size));
> >>>+
> >>>+    SCTP_DBG_OBJCNT_DEC(ssnmap);
> >>>  }
> >>>--
> >>>1.7.11.7
> >>>
> >>I definately like what you're doing here, as the use of the ->malloced member
> >>always struck me as a half-assed way to try and avoid a double free that someone
> >>couldn't track down during this code's initial development.  That said, I'm
> >>wondering if the !map check is going to fail at some point, given that the call
> >>site for sctp_ssnmap_free never sets asoc->ssnmap to NULL after its call.  Maybe
> >>worthwhile adding such a NULL assoginment to the call site to ensure that we
> >>don't accidentally trigger a double free?
> >
> >I'll test that with lksctp-tools suite and come back to you today.
> 
> Just did that.
> 
> I've poisoned the pointers, so that they would throw a WARN_ON() if they have
> already been seen. Also, I've put a WARN_ON() before sctp_ssnmap_new() in
> sctp_process_init(), in case asoc->ssnmap was not NULL. I've run the lksctp-tools
> suite for v4/v6 and nothing was thrown, also it all passed.
> 
> That said, I think that the !map check is there because we init the asoc first
> with a NULL ssnmap. I suggest, if Dave wants to and if there are no other
> objections, that we could apply to net-next the patches ...
> 
>   * [1/9] http://patchwork.ozlabs.org/patch/237101/
>   * [2/9] http://patchwork.ozlabs.org/patch/237102/
>   * [3/9] http://patchwork.ozlabs.org/patch/237103/
>   * [5/9] http://patchwork.ozlabs.org/patch/237105/
>   * [6/9] http://patchwork.ozlabs.org/patch/237109/
>   * [7/9] http://patchwork.ozlabs.org/patch/237106/
>   * [8/9] http://patchwork.ozlabs.org/patch/237107/
> 
> ... as is. I've just tested it, they apply cleanly on top of each other without
> the missing. Alternatively, I could resend the set without the two that we cut
> out (nr 4 and 9). How you prefer, let me know.
> 
Dave's the final word, but I think the typical workflow is a resend to the list.

Neil
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html