| 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: <d8d357e5-45e7-87ae-ad87-53e9dc6ce5c2@gmail.com>
Date: Tue, 4 Dec 2018 19:51:54 +0100
From: Jakub Audykowicz <jakub.audykowicz@...il.com>
To: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc: Xin Long <lucien.xin@...il.com>, linux-sctp@...r.kernel.org,
Vlad Yasevich <vyasevich@...il.com>,
Neil Horman <nhorman@...driver.com>,
davem <davem@...emloft.net>, network dev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] sctp: always set frag_point on pmtu change
On 2018-12-04 18:45, Marcelo Ricardo Leitner wrote:
> On Tue, Dec 04, 2018 at 06:00:51PM +0100, Jakub Audykowicz wrote:
> ...
>> OK, let's forget about that "if" :)
>> Coming back to the sanity check, I came up with something like below,
>> based on the code from sctp_setsockopt_maxseg, like you mentioned.
>> I may have overcomplicated things since I didn't know how to accomplish
>> the same thing without passing sctp_sock* to sctp_datamsg_from_user.
> Yep. More below.
>
>> I wanted to avoid calling sctp_min_frag_point unless absolutely
>> necessary, so I just check the frag_point against the zero that is
>> causing the eventual kernel panic.
> Makes sense.
>
>>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index ab9242e51d9e..7e67c0257b3f 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -620,4 +620,15 @@ static inline bool sctp_transport_pmtu_check(struct sctp_transport *t)
>> return false;
>> }
>>
>> +static inline __u16 sctp_data_chunk_size(struct sctp_association *asoc)
>> +{
>> + return asoc ? sctp_datachk_len(&asoc->stream) :
>> + sizeof(struct sctp_data_chunk);
> I don't think we need another layer on top of data chunk sizes here.
> We don't need this asoc check by the sendmsg callpath because it
> cannot be NULL by then. That said, I think we have avoid this helper,
> for now at least.
>
> One other way would be to include the check into sctp_datachk_len(),
> but we currently have 9 calls to that but only 1 requires the check.
>
> As asoc is initialized and considering the fix we just had in this
> area, stream->si will also be initialized so you should be good to
> just call sctp_datachk_len() directly.
>
>> +}
>> +
>> +static inline __u32 sctp_min_frag_point(struct sctp_sock *sp, __u16 datasize)
>> +{
>> + return sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT, datasize);
>> +}
> This is a good helper to have. Makes it clearer.
>
>> +
>> #endif /* __net_sctp_h__ */
>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>> index a11f93790476..d09b5de73c92 100644
>> --- a/include/net/sctp/structs.h
>> +++ b/include/net/sctp/structs.h
>> @@ -543,7 +543,8 @@ struct sctp_datamsg {
>>
>> struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
>> struct sctp_sndrcvinfo *,
>> - struct iov_iter *);
>> + struct iov_iter *,
>> + struct sctp_sock *);
>> void sctp_datamsg_free(struct sctp_datamsg *);
>> void sctp_datamsg_put(struct sctp_datamsg *);
>> void sctp_chunk_fail(struct sctp_chunk *, int error);
>> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
>> index ce8087846f05..753c2c123767 100644
>> --- a/net/sctp/chunk.c
>> +++ b/net/sctp/chunk.c
>> @@ -164,7 +164,8 @@ static void sctp_datamsg_assign(struct sctp_datamsg *msg, struct sctp_chunk *chu
>> */
>> struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>> struct sctp_sndrcvinfo *sinfo,
>> - struct iov_iter *from)
>> + struct iov_iter *from,
>> + struct sctp_sock *sp)
>> {
>> size_t len, first_len, max_data, remaining;
>> size_t msg_len = iov_iter_count(from);
>> @@ -173,6 +174,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>> struct sctp_chunk *chunk;
>> struct sctp_datamsg *msg;
>> int err;
>> + __u32 min_frag_point;
> Reverse christmass tree.. swap the last two lines:
> + __u32 min_frag_point;
> int err;
> But I guess we don't need this var anymore:
>
>>
>> msg = sctp_datamsg_new(GFP_KERNEL);
>> if (!msg)
>> @@ -190,6 +192,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>> /* This is the biggest possible DATA chunk that can fit into
>> * the packet
>> */
>> + if (unlikely(asoc->frag_point == 0)) {
> !asoc->frag_point instead
>
> You can get to sctp_sock here with: sctp_sk(asoc->base.sk)
> struct sctp_sock *sp = sctp_sk(asoc->base.sk);
>
>> + min_frag_point = sctp_min_frag_point(sp, sctp_data_chunk_size(asoc));
>> + pr_warn_ratelimited("%s: asoc:%p frag_point is too low (%d < %d), using default minimum",
>> + __func__, asoc, asoc->frag_point, min_frag_point);
>> + asoc->frag_point = min_frag_point;
> No no. Lets not fix up things here. If we do this assignment, we may
> make the disparity even worse.
> With that, we can work only on max_data and avoid the need of min_frag_point.
> max_data = asoc->frag_point;
> if (unlikely(!max_data)) {
> ...
> max_data = ...
> }
>
>> + }
>> max_data = asoc->frag_point;
>>
>> /* If the the peer requested that we authenticate DATA chunks
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index bf618d1b41fd..28d711609ef1 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -1938,7 +1938,7 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
>> pr_debug("%s: we associated primitively\n", __func__);
>> }
>>
>> - datamsg = sctp_datamsg_from_user(asoc, sinfo, &msg->msg_iter);
>> + datamsg = sctp_datamsg_from_user(asoc, sinfo, &msg->msg_iter, sp);
>> if (IS_ERR(datamsg)) {
>> err = PTR_ERR(datamsg);
>> goto err;
>> @@ -3321,11 +3321,9 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
>>
>> if (val) {
>> int min_len, max_len;
>> - __u16 datasize = asoc ? sctp_datachk_len(&asoc->stream) :
>> - sizeof(struct sctp_data_chunk);
>> + __u16 datasize = sctp_data_chunk_size(asoc);
>>
>> - min_len = sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT,
>> - datasize);
>> + min_len = sctp_min_frag_point(sp, datasize);
>> max_len = SCTP_MAX_CHUNK_LEN - datasize;
>>
>> if (val < min_len || val > max_len)
>>
Thanks, I've taken your remarks into account and ended up with this
simple solution:
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index ab9242e51d9e..3487686f2cf5 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -620,4 +620,9 @@ static inline bool sctp_transport_pmtu_check(struct sctp_transport *t)
return false;
}
+static inline __u32 sctp_min_frag_point(struct sctp_sock *sp, __u16 datasize)
+{
+ return sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT, datasize);
+}
+
#endif /* __net_sctp_h__ */
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index ce8087846f05..dc12c2ba487f 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -191,6 +191,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
* the packet
*/
max_data = asoc->frag_point;
+ if (unlikely(!max_data)) {
+ pr_warn_ratelimited("%s: asoc:%p frag_point is zero, forcing max_data to default minimum",
+ __func__, asoc);
+ max_data = sctp_min_frag_point(
+ sctp_sk(asoc->base.sk), sctp_datachk_len(&asoc->stream));
+ }
/* If the the peer requested that we authenticate DATA chunks
* we need to account for bundling of the AUTH chunks along with
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index bf618d1b41fd..b8cebd5a87e5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3324,8 +3324,7 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
__u16 datasize = asoc ? sctp_datachk_len(&asoc->stream) :
sizeof(struct sctp_data_chunk);
- min_len = sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT,
- datasize);
+ min_len = sctp_min_frag_point(sp, datasize);
max_len = SCTP_MAX_CHUNK_LEN - datasize;
if (val < min_len || val > max_len)
Powered by blists - more mailing lists