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: <5089B237.6010902@gmail.com> Date: Thu, 25 Oct 2012 17:42:15 -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/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 > } > static inline sctp_arg_t SCTP_NOFORCE(void) > { > @@ -181,7 +183,6 @@ SCTP_ARG_CONSTRUCTOR(ERROR, int, error) > SCTP_ARG_CONSTRUCTOR(PERR, __be16, err) /* protocol error */ > SCTP_ARG_CONSTRUCTOR(STATE, sctp_state_t, state) > SCTP_ARG_CONSTRUCTOR(TO, sctp_event_timeout_t, to) > -SCTP_ARG_CONSTRUCTOR(PTR, void *, ptr) > SCTP_ARG_CONSTRUCTOR(CHUNK, struct sctp_chunk *, chunk) > SCTP_ARG_CONSTRUCTOR(ASOC, struct sctp_association *, asoc) > SCTP_ARG_CONSTRUCTOR(TRANSPORT, struct sctp_transport *, transport) > diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h > index 2e5ee0d..ff1b8ba7 100644 > --- a/include/net/sctp/ulpqueue.h > +++ b/include/net/sctp/ulpqueue.h > @@ -72,7 +72,7 @@ int sctp_ulpq_tail_event(struct sctp_ulpq *, struct sctp_ulpevent *ev); > void sctp_ulpq_renege(struct sctp_ulpq *, struct sctp_chunk *, gfp_t); > > /* Perform partial delivery. */ > -void sctp_ulpq_partial_delivery(struct sctp_ulpq *, struct sctp_chunk *, gfp_t); > +void sctp_ulpq_partial_delivery(struct sctp_ulpq *, gfp_t); > > /* Abort the partial delivery. */ > void sctp_ulpq_abort_pd(struct sctp_ulpq *, gfp_t); > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c > index 6773d78..6eecf7e 100644 > --- a/net/sctp/sm_sideeffect.c > +++ b/net/sctp/sm_sideeffect.c > @@ -1268,14 +1268,14 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > sctp_outq_uncork(&asoc->outqueue); > local_cork = 0; > } > - asoc = cmd->obj.ptr; > + asoc = cmd->obj.asoc; > /* Register with the endpoint. */ > sctp_endpoint_add_asoc(ep, asoc); > sctp_hash_established(asoc); > break; > > case SCTP_CMD_UPDATE_ASSOC: > - sctp_assoc_update(asoc, cmd->obj.ptr); > + sctp_assoc_update(asoc, cmd->obj.asoc); > break; > > case SCTP_CMD_PURGE_OUTQUEUE: > @@ -1315,7 +1315,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > break; > > case SCTP_CMD_PROCESS_FWDTSN: > - sctp_cmd_process_fwdtsn(&asoc->ulpq, cmd->obj.ptr); > + sctp_cmd_process_fwdtsn(&asoc->ulpq, cmd->obj.chunk); > break; > > case SCTP_CMD_GEN_SACK: > @@ -1331,7 +1331,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > case SCTP_CMD_PROCESS_SACK: > /* Process an inbound SACK. */ > error = sctp_cmd_process_sack(commands, asoc, > - cmd->obj.ptr); > + cmd->obj.chunk); > break; > > case SCTP_CMD_GEN_INIT_ACK: > @@ -1352,15 +1352,15 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > * layer which will bail. > */ > error = sctp_cmd_process_init(commands, asoc, chunk, > - cmd->obj.ptr, gfp); > + cmd->obj.init, gfp); > break; > > case SCTP_CMD_GEN_COOKIE_ECHO: > /* Generate a COOKIE ECHO chunk. */ > new_obj = sctp_make_cookie_echo(asoc, chunk); > if (!new_obj) { > - if (cmd->obj.ptr) > - sctp_chunk_free(cmd->obj.ptr); > + if (cmd->obj.chunk) > + sctp_chunk_free(cmd->obj.chunk); > goto nomem; > } > sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, > @@ -1369,9 +1369,9 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > /* If there is an ERROR chunk to be sent along with > * the COOKIE_ECHO, send it, too. > */ > - if (cmd->obj.ptr) > + if (cmd->obj.chunk) > sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, > - SCTP_CHUNK(cmd->obj.ptr)); > + SCTP_CHUNK(cmd->obj.chunk)); > > if (new_obj->transport) { > new_obj->transport->init_sent_count++; > @@ -1417,18 +1417,18 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > case SCTP_CMD_CHUNK_ULP: > /* Send a chunk to the sockets layer. */ > SCTP_DEBUG_PRINTK("sm_sideff: %s %p, %s %p.\n", > - "chunk_up:", cmd->obj.ptr, > + "chunk_up:", cmd->obj.chunk, > "ulpq:", &asoc->ulpq); > - sctp_ulpq_tail_data(&asoc->ulpq, cmd->obj.ptr, > + sctp_ulpq_tail_data(&asoc->ulpq, cmd->obj.chunk, > GFP_ATOMIC); > break; > > case SCTP_CMD_EVENT_ULP: > /* Send a notification to the sockets layer. */ > SCTP_DEBUG_PRINTK("sm_sideff: %s %p, %s %p.\n", > - "event_up:",cmd->obj.ptr, > + "event_up:",cmd->obj.ulpevent, > "ulpq:",&asoc->ulpq); > - sctp_ulpq_tail_event(&asoc->ulpq, cmd->obj.ptr); > + sctp_ulpq_tail_event(&asoc->ulpq, cmd->obj.ulpevent); > break; > > case SCTP_CMD_REPLY: > @@ -1438,12 +1438,12 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > local_cork = 1; > } > /* Send a chunk to our peer. */ > - error = sctp_outq_tail(&asoc->outqueue, cmd->obj.ptr); > + error = sctp_outq_tail(&asoc->outqueue, cmd->obj.chunk); > break; > > case SCTP_CMD_SEND_PKT: > /* Send a full packet to our peer. */ > - packet = cmd->obj.ptr; > + packet = cmd->obj.packet; > sctp_packet_transmit(packet); > sctp_ootb_pkt_free(packet); > break; > @@ -1480,7 +1480,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > break; > > case SCTP_CMD_SETUP_T2: > - sctp_cmd_setup_t2(commands, asoc, cmd->obj.ptr); > + sctp_cmd_setup_t2(commands, asoc, cmd->obj.chunk); > break; > > case SCTP_CMD_TIMER_START_ONCE: > @@ -1514,7 +1514,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > break; > > case SCTP_CMD_INIT_CHOOSE_TRANSPORT: > - chunk = cmd->obj.ptr; > + chunk = cmd->obj.chunk; > t = sctp_assoc_choose_alter_transport(asoc, > asoc->init_last_sent_to); > asoc->init_last_sent_to = t; > @@ -1665,17 +1665,16 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > break; > > case SCTP_CMD_PART_DELIVER: > - sctp_ulpq_partial_delivery(&asoc->ulpq, cmd->obj.ptr, > - GFP_ATOMIC); > + sctp_ulpq_partial_delivery(&asoc->ulpq, GFP_ATOMIC); > break; > > case SCTP_CMD_RENEGE: > - sctp_ulpq_renege(&asoc->ulpq, cmd->obj.ptr, > + sctp_ulpq_renege(&asoc->ulpq, cmd->obj.chunk, > GFP_ATOMIC); > break; > > case SCTP_CMD_SETUP_T4: > - sctp_cmd_setup_t4(commands, asoc, cmd->obj.ptr); > + sctp_cmd_setup_t4(commands, asoc, cmd->obj.chunk); > break; > > case SCTP_CMD_PROCESS_OPERR: > @@ -1734,8 +1733,8 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, > break; > > default: > - pr_warn("Impossible command: %u, %p\n", > - cmd->verb, cmd->obj.ptr); > + pr_warn("Impossible command: %u\n", > + cmd->verb); > break; > } > > diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c > index 360d869..ada1746 100644 > --- a/net/sctp/ulpqueue.c > +++ b/net/sctp/ulpqueue.c > @@ -997,7 +997,6 @@ static __u16 sctp_ulpq_renege_frags(struct sctp_ulpq *ulpq, __u16 needed) > > /* Partial deliver the first message as there is pressure on rwnd. */ > void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq, > - struct sctp_chunk *chunk, > gfp_t gfp) > { > struct sctp_ulpevent *event; > @@ -1060,7 +1059,7 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk, > sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport); > sctp_ulpq_tail_data(ulpq, chunk, gfp); > > - sctp_ulpq_partial_delivery(ulpq, chunk, gfp); > + sctp_ulpq_partial_delivery(ulpq, gfp); > } > > sk_mem_reclaim(asoc->base.sk); > -- 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