| 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: <20170120114352.GQ3781@localhost.localdomain>
Date: Fri, 20 Jan 2017 09:43:53 -0200
From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To: Xin Long <lucien.xin@...il.com>
Cc: network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
Neil Horman <nhorman@...driver.com>,
Vlad Yasevich <vyasevich@...il.com>,
davem <davem@...emloft.net>
Subject: Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures
for Add Incoming/Outgoing Streams Request Parameter
On Fri, Jan 20, 2017 at 04:56:30PM +0800, Xin Long wrote:
> On Fri, Jan 20, 2017 at 5:47 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@...il.com> wrote:
> > On Fri, Jan 20, 2017 at 01:19:14AM +0800, Xin Long wrote:
> >> This patch is to implement Sender-Side Procedures for the Add
> >> Outgoing and Incoming Streams Request Parameter described in
> >> rfc6525 section 5.1.5-5.1.6.
> >>
> >> It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> >> 6.3.4 for users.
> >>
> >> Signed-off-by: Xin Long <lucien.xin@...il.com>
> >> ---
> >> include/net/sctp/sctp.h | 2 +
> >> include/uapi/linux/sctp.h | 7 ++++
> >> net/sctp/socket.c | 29 ++++++++++++++
> >> net/sctp/stream.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++
> >> 4 files changed, 137 insertions(+)
> >>
> >> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> >> index b93820f..68ee1a6 100644
> >> --- a/include/net/sctp/sctp.h
> >> +++ b/include/net/sctp/sctp.h
> >> @@ -199,6 +199,8 @@ int sctp_offload_init(void);
> >> int sctp_send_reset_streams(struct sctp_association *asoc,
> >> struct sctp_reset_streams *params);
> >> int sctp_send_reset_assoc(struct sctp_association *asoc);
> >> +int sctp_send_add_streams(struct sctp_association *asoc,
> >> + struct sctp_add_streams *params);
> >>
> >> /*
> >> * Module global variables
> >> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> >> index c0bd8c3..a91a9cc 100644
> >> --- a/include/uapi/linux/sctp.h
> >> +++ b/include/uapi/linux/sctp.h
> >> @@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
> >> #define SCTP_ENABLE_STREAM_RESET 118
> >> #define SCTP_RESET_STREAMS 119
> >> #define SCTP_RESET_ASSOC 120
> >> +#define SCTP_ADD_STREAMS 121
> >>
> >> /* PR-SCTP policies */
> >> #define SCTP_PR_SCTP_NONE 0x0000
> >> @@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
> >> uint16_t srs_stream_list[]; /* list if srs_num_streams is not 0 */
> >> };
> >>
> >> +struct sctp_add_streams {
> >> + sctp_assoc_t sas_assoc_id;
> >> + uint16_t sas_instrms;
> >> + uint16_t sas_outstrms;
> >> +};
> >> +
> >> #endif /* _UAPI_SCTP_H */
> >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >> index 2c5c9ca..ae0a99e 100644
> >> --- a/net/sctp/socket.c
> >> +++ b/net/sctp/socket.c
> >> @@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
> >> return retval;
> >> }
> >>
> >> +static int sctp_setsockopt_add_streams(struct sock *sk,
> >> + char __user *optval,
> >> + unsigned int optlen)
> >> +{
> >> + struct sctp_association *asoc;
> >> + struct sctp_add_streams params;
> >> + int retval = -EINVAL;
> >> +
> >> + if (optlen != sizeof(params))
> >> + goto out;
> >> +
> >> + if (copy_from_user(¶ms, optval, optlen)) {
> >> + retval = -EFAULT;
> >> + goto out;
> >> + }
> >> +
> >> + asoc = sctp_id2assoc(sk, params.sas_assoc_id);
> >> + if (!asoc)
> >> + goto out;
> >> +
> >> + retval = sctp_send_add_streams(asoc, ¶ms);
> >> +
> >> +out:
> >> + return retval;
> >> +}
> >> +
> >> /* API 6.2 setsockopt(), getsockopt()
> >> *
> >> * Applications use setsockopt() and getsockopt() to set or retrieve
> >> @@ -4013,6 +4039,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
> >> case SCTP_RESET_ASSOC:
> >> retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
> >> break;
> >> + case SCTP_ADD_STREAMS:
> >> + retval = sctp_setsockopt_add_streams(sk, optval, optlen);
> >> + break;
> >> default:
> >> retval = -ENOPROTOOPT;
> >> break;
> >> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> >> index b368191..ba41837 100644
> >> --- a/net/sctp/stream.c
> >> +++ b/net/sctp/stream.c
> >> @@ -195,3 +195,102 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
> >>
> >> return retval;
> >> }
> >> +
> >> +int sctp_send_add_streams(struct sctp_association *asoc,
> >> + struct sctp_add_streams *params)
> >> +{
> >> + struct sctp_stream *stream = asoc->stream;
> >> + struct sctp_chunk *chunk = NULL;
> >> + int retval = -EINVAL;
> >> + __u16 out, in;
> >> +
> >> + if (!asoc->peer.reconf_capable ||
> >> + !(asoc->strreset_enable & SCTP_ENABLE_CHANGE_ASSOC_REQ)) {
> >> + retval = -ENOPROTOOPT;
> >> + goto out;
> >> + }
> >> +
> >> + if (asoc->strreset_outstanding) {
> >> + retval = -EINPROGRESS;
> >> + goto out;
> >> + }
> >> +
> >> + out = params->sas_outstrms;
> >> + in = params->sas_instrms;
> >> +
> >> + if (!out && !in)
> >
> > [@]
> >
> >> + goto out;
> >> +
> >> + if (out) {
> >> + __u16 nums = stream->outcnt + out;
> >> +
> >> + /* Check for overflow, can't use nums here */
> >> + if (stream->outcnt + out > SCTP_MAX_STREAM)
> >> + goto out;
> >
> > You should do these overflow checks before doing actual work,
> > preferreably at [@] mark above.
> > Here it's fine, but when [*] below run, it may be too late.
> >
> >> +
> >> + /* Use ksize to check if stream array really needs to realloc */
> >> + if (ksize(stream->out) / sizeof(*stream->out) < nums) {
> >> + struct sctp_stream_out *streamout;
> >> +
> >> + streamout = kcalloc(nums, sizeof(*streamout),
> >> + GFP_KERNEL);
> >> + if (!streamout) {
> >> + retval = -ENOMEM;
> >> + goto out;
> >> + }
> >> +
> >> + memcpy(streamout, stream->out,
> >> + sizeof(*streamout) * stream->outcnt);
> >> +
> >> + kfree(stream->out);
> >> + stream->out = streamout;
> >> + }
> >> +
> >> + stream->outcnt = nums;
> >> + }
> >> +
> >> + if (in) {
> >> + __u16 nums = stream->incnt + in;
> >> +
> >> + if (stream->incnt + in > SCTP_MAX_STREAM)
> >> + goto out;
> >
> > [*]
> it will cause alloc unused memory, but no consistent issue if
> we move outcnt/incnt after checking retval.
Ah yes, right, so it doesn't hurt actually.
>
> if we want overflow check to go ahead of the actual work, another
> conditions if (in) {} if(out) {} have to be added.
Not sure why? Seems
if (!in || !out || stream->incnt + in > SCTP_MAX_STREAM ||
stream->outcnt + out > SCTP_MAX_STREAM)
goto out;
would do it, no?
>
> >
> >> +
> >> + if (ksize(stream->in) / sizeof(*stream->in) < nums) {
> >> + struct sctp_stream_in *streamin;
> >> +
> >> + streamin = kcalloc(nums, sizeof(*streamin),
> >> + GFP_KERNEL);
> >> + if (!streamin) {
> >> + retval = -ENOMEM;
> >> + goto out;
> >> + }
> >> +
> >> + memcpy(streamin, stream->in,
> >> + sizeof(*streamin) * stream->incnt);
> >> +
> >> + kfree(stream->in);
> >> + stream->in = streamin;
> >> + }
> >> +
> >> + stream->incnt = nums;
> >> + }
> >> +
> >> + chunk = sctp_make_strreset_addstrm(asoc, out, in);
> >> + if (!chunk) {
> >> + retval = -ENOMEM;
> >> + goto out;
> >> + }
> >> +
> >> + asoc->strreset_outstanding = !!out + !!in;
> >> + asoc->strreset_chunk = chunk;
> >> + sctp_chunk_hold(asoc->strreset_chunk);
> >> +
> >> + retval = sctp_send_reconf(asoc, chunk);
> >> + if (retval) {
> >> + sctp_chunk_put(asoc->strreset_chunk);
> >> + asoc->strreset_chunk = NULL;
> >> + }
> >> +
> >> +out:
> >> + return retval;
> >> +}
> >> --
> >> 2.1.0
> >>
> >> --
> >> 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
> >>
> --
> 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
>
Powered by blists - more mailing lists