| 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
| ||
|
Message-ID: <508AE08B.8070303@gmail.com>
Date: Fri, 26 Oct 2012 15:12:11 -0400
From: Vlad Yasevich <vyasevich@...il.com>
To: Neil Horman <nhorman@...driver.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 10/26/2012 09:24 AM, Neil Horman wrote:
> 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?
>
Look at commit 19c7e9ee that introduced this. I don't remember all the
details any more, but the problem only occurred on ia64 (probably due
its speculative load handling).
-vlad
> 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
Powered by blists - more mailing lists