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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 17 Apr 2013 08:45:39 -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
Subject: Re: [PATCH net-next 1/9] net: sctp: sctp_ssnmap: remove 'malloced'
 element from struct

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?

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