netdev - Re: [PATCH net 1/6] sctp: remove the unnecessary state check in sctp_outq

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, 8 Sep 2016 15:23:19 -0400
From:   Neil Horman <nhorman@...driver.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem <davem@...emloft.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Vlad Yasevich <vyasevich@...il.com>, daniel@...earbox.net
Subject: Re: [PATCH net 1/6] sctp: remove the unnecessary state check in
 sctp_outq_tail

On Fri, Sep 09, 2016 at 01:34:05AM +0800, Xin Long wrote:
> >> Data Chunks are only sent by sctp_primitive_SEND, in which sctp checks
> >> the asoc's state through statetable before calling sctp_outq_tail. So
> >> there's no need to do it again in sctp_outq_tail.
> >>
> >> This patch is to remove it from sctp_outq_tail.
> >>
> >> Signed-off-by: Xin Long <lucien.xin@...il.com>
> > This doesn't seem safe to me.  The send operation is handled in side effect
> > processing off a queue that might have operations queued ahead of it, which
> > affect the associations state.  I think you need to keep this check in place
> >
> But not really for Data Chunk.
> 
> There are 3 places calling sctp_outq_tail.
> 1. in sctp_assoc_rwnd_increase():
>    it's for sending sack chunk, so this patch is safe for here
> 2. sctp_cmd_interpreter(), SCTP_CMD_REPLY:
>    still not Data Chunk, so safe.
> 3.SCTP_CMD_SEND_MSG: ->  sctp_cmd_send_msg()
>    it's for Data Chunk, but it's only called by sctp_sf_do_prm_send().
> 
Its not enough to just look at the paths where outq_tail is called, because
the outq_tail function is checking a state variable that can update
asynchronously in the side effect processing queue.  Look at any one of the
timer functions.  Those all fire asynchronous to the outq list, but they all
enqueue to the side effect list.  Its entirely possible that the state can be
ESTABLISHED in the primitive send path, but before you enqueue the SEND
sideffect, a timer will pop and enqueue a NEW_STATE side effect.  The end result
is you need to check the state again.

Neil

> #define TYPE_SCTP_PRIMITIVE_SEND  { \
> ...
> /* SCTP_STATE_COOKIE_WAIT */ \
> TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \
> /* SCTP_STATE_COOKIE_ECHOED */ \
> TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \
> /* SCTP_STATE_ESTABLISHED */ \
> TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \
> /* SCTP_STATE_SHUTDOWN_PENDING */ \
> 
> 
> in sctp_sf_do_prm_send():
>    sctp_add_cmd_sf(commands, SCTP_CMD_SEND_MSG, SCTP_DATAMSG(msg));
>    return SCTP_DISPOSITION_CONSUME;
> 
> This function is called by PRIMITIVE_SEND, and every time after
> calling sctp_do_sm(), there must be no operations inside.
> so it's impossible that other operations queued ahead of it.
> 
> I'm not sure if I miss some special case, pls correct me if I'm wrong.
> 
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>