netdev - Re: [PATCH] sctp: Clean up type-punning in sctp_cmd

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:	Thu, 25 Oct 2012 19:58:43 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Vlad Yasevich <vyasevich@...il.com>
Cc:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	linux-sctp@...r.kernel.org
Subject: Re: [PATCH] sctp: Clean up type-punning in sctp_cmd_t union

On Thu, Oct 25, 2012 at 05:42:15PM -0400, Vlad Yasevich wrote:
> On 10/25/2012 04:47 PM, Neil Horman wrote:
> >Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
> >a void pointer, even though they are written as various other types.  Theres no
> >need for this as doing so just leads to possible type-punning issues that could
> >cause crashes, and if we remain type-consistent we can actually just remove the
> >void * member of the union entirely.
> >
> >Signed-off-by: Neil Horman <nhorman@...driver.com
> >CC: Vlad Yasevich <vyasevich@...il.com>
> >CC: "David S. Miller" <davem@...emloft.net>
> >CC: linux-sctp@...r.kernel.org
> >---
> >  include/net/sctp/command.h  |  7 ++++---
> >  include/net/sctp/ulpqueue.h |  2 +-
> >  net/sctp/sm_sideeffect.c    | 45 ++++++++++++++++++++++-----------------------
> >  net/sctp/ulpqueue.c         |  3 +--
> >  4 files changed, 28 insertions(+), 29 deletions(-)
> >
> >diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
> >index 712b3be..7f1b0f3 100644
> >--- a/include/net/sctp/command.h
> >+++ b/include/net/sctp/command.h
> >@@ -131,7 +131,6 @@ typedef union {
> >  	sctp_state_t state;
> >  	sctp_event_timeout_t to;
> >  	unsigned long zero;
> >-	void *ptr;
> >  	struct sctp_chunk *chunk;
> >  	struct sctp_association *asoc;
> >  	struct sctp_transport *transport;
> >@@ -154,9 +153,12 @@ typedef union {
> >   * which takes an __s32 and returns a sctp_arg_t containing the
> >   * __s32.  So, after foo = SCTP_I32(arg), foo.i32 == arg.
> >   */
> >+#define SCTP_NULL_BYTE 0xAA
> >  static inline sctp_arg_t SCTP_NULL(void)
> >  {
> >-	sctp_arg_t retval; retval.ptr = NULL; return retval;
> >+	sctp_arg_t retval;
> >+	memset(&retval, SCTP_NULL_BYTE, sizeof(sctp_arg_t));
> >+	return retval;
> 
> What's this for?  Can't we just use retval.zero?
> 
> -vlad
> 
My intent was to highlight any users of sctp_arg_t when SCTP_NULL was passed.
My thinking was that the 0xAA byte patern would be a good indicator.  Although,
admittedly I didn't see the zero argument there.  Looking at it though, the zero
member of the union is effectively unused.  Strictly speaking its used for
initalization of sctp_arg_t, but its done somewhat poorly, since theres no
guarantee that an unsigned long will be the largest member of that union.  Doing
the memset guarantees the whole instance is set to a predefined value.

I could go either way with this, would you rather we just have SCTP_NULL return
retval = { .zero = 0}; or would you rather remove the zero initialization from
SCTP_[NO]FORCE, and SCTP_ARG_CONSTRUCTOR and do the memset.  I think the memset
reduces to a single 64 bit assignment as long as the union doesn't exceed that
size anyway, and it ensures that you initalize the whole union's storage if it
does in the future.  And if we remove the initialization step (I don't see that
its needed in the three macros above anyway), then we can remove the zero member
as well.

Let me know what you want to do here, and I can respin this.
Best
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