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 for Android: free password hash cracker in your pocket

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

Date:	Fri, 26 Oct 2012 09:24:22 -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 11:48:16PM -0400, Vlad Yasevich wrote:
> On 10/25/2012 07:58 PM, Neil Horman wrote:
> >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.
> >
> 
> You need the initialization step, otherwise things might fail (they
> did on IA64 a while back).  That's why the zero member was added.
> You can go with memset if you want, but I was primarily wondering
> why the 0xAA pattern was there.
> 
The AA I did was just meant as a pattern marker, so that, should someone use an
instance of sctp_arg_t that was passed in as SCTP_NULL(), it would be visually
obvious in the stack trace, but I suppose its not really needed given that NULL
is equally clear.  And since Dave pointed out the lack of optimization
opportunity when using a store to an address rather than a register, I think I
should probably just revert it and use zero as you initially suggested.

The need for the initalization in SCTP_[NO]FORCE and SCTP_ARG_CONSTRUCTOR
concerns me though.  All its doing is setting part of the storage to zero, and
then overwriting it again with whatever type spcific member you're assigning
from the corresponding SCTP_* macro.  That kind of sounds to me like ia64 might
have fallen to some amount of type-punning problem.  do you have a link to
discussion about that problem?

Regards
Neil

> -vlad
> >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