On 2018-11-28 12:26, Marcelo Ricardo Leitner wrote:
> On Wed, Nov 28, 2018 at 12:08:38AM -0200, Marcelo Ricardo Leitner wrote:
>> On Tue, Nov 27, 2018 at 11:18:02PM +0100, Jakub Audykowicz wrote:
>>> On 2018-11-19 08:20, Xin Long wrote:
>>>
>>>> On Mon, Nov 19, 2018 at 5:49 AM Jakub Audykowicz
>>>> <jakub.audykowicz@xxxxxxxxx> wrote:
>>>>> Calling send on a connected SCTP socket results in kernel panic if
>>>>> spp_pathmtu was configured manually before an association is established
>>>>> and it was not reconfigured to another value once the association is
>>>>> established.
>>>>>
>>>>> Steps to reproduce:
>>>>> 1. Set up a listening SCTP server socket.
>>>>> 2. Set up an SCTP client socket.
>>>>> 3. Configure client socket using setsockopt SCTP_PEER_ADDR_PARAMS with
>>>>> spp_pathmtu set to a legal value (e.g. 1000) and
>>>>> SPP_PMTUD_DISABLE set in spp_flags.
>>>>> 4. Connect client to server.
>>>>> 5. Send message from client to server.
>>>>>
>>>>> At this point oom-killer is invoked, which will eventually lead to:
>>>>> [ 5.197262] Out of memory and no killable processes...
>>>>> [ 5.198107] Kernel panic - not syncing: System is deadlocked on memory
>>>>>
>>>>> Commit 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
>>>>> introduces sctp_assoc_update_frag_point, but this function is not called
>>>>> in this case, causing frag_point to be zero:
>>>>> void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
>>>>> {
>>>>> - if (asoc->pathmtu != pmtu)
>>>>> + if (asoc->pathmtu != pmtu) {
>>>>> asoc->pathmtu = pmtu;
>>>>> + sctp_assoc_update_frag_point(asoc);
>>>>> + }
>>>>>
>>>>> In this scenario, on association establishment, asoc->pathmtu is already
>>>>> 1000 and pmtu will be as well. Before this commit the frag_point was being
>>>>> correctly set in the scenario described. Moving the call outside the if
>>>>> block fixes the issue.
>>>>>
>>>>> I will be providing a separate patch to lksctp-tools with a simple test
>>>>> reproducing this problem ("func_tests: frag_point should never be zero").
>>>>>
>>>>> I have also taken the liberty to introduce a sanity check in chunk.c to
>>>>> set the frag_point to a non-negative value in order to avoid chunking
>>>>> endlessly (which is the reason for the eventual panic).
>>>>>
>>>>> Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
>>>>> Signed-off-by: Jakub Audykowicz <jakub.audykowicz@xxxxxxxxx>
>>>>> ---
>>>>> include/net/sctp/constants.h | 3 +++
>>>>> net/sctp/associola.c | 13 +++++++------
>>>>> net/sctp/chunk.c | 6 ++++++
>>>>> 3 files changed, 16 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>>>> index 8dadc74c22e7..90316fab6f04 100644
>>>>> --- a/include/net/sctp/constants.h
>>>>> +++ b/include/net/sctp/constants.h
>>>>> @@ -293,6 +293,9 @@ enum { SCTP_MAX_GABS = 16 };
>>>>> */
>>>>> #define SCTP_DEFAULT_MINSEGMENT 512 /* MTU size ... if no mtu disc */
>>>>>
>>>>> +/* An association's fragmentation point should never be non-positive */
>>>>> +#define SCTP_FRAG_POINT_MIN 1
>>>>> +
>>>>> #define SCTP_SECRET_SIZE 32 /* Number of octets in a 256 bits. */
>>>>>
>>>>> #define SCTP_SIGNATURE_SIZE 20 /* size of a SLA-1 signature */
>>>>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>>>>> index 6a28b96e779e..44d71a1af62e 100644
>>>>> --- a/net/sctp/associola.c
>>>>> +++ b/net/sctp/associola.c
>>>>> @@ -1431,13 +1431,14 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
>>>>>
>>>>> void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
>>>>> {
>>>>> - if (asoc->pathmtu != pmtu) {
>>>>> - asoc->pathmtu = pmtu;
>>>>> - sctp_assoc_update_frag_point(asoc);
>>>>> - }
>>>>> + pr_debug("%s: before asoc:%p, pmtu:%d, frag_point:%d\n",
>>>>> + __func__, asoc, asoc->pathmtu, asoc->frag_point);
>>>>> +
>>>>> + asoc->pathmtu = pmtu;
>>>>> + sctp_assoc_update_frag_point(asoc);
>>>>>
>>>>> - pr_debug("%s: asoc:%p, pmtu:%d, frag_point:%d\n", __func__, asoc,
>>>>> - asoc->pathmtu, asoc->frag_point);
>>>>> + pr_debug("%s: after asoc:%p, pmtu:%d, frag_point:%d\n",
>>>>> + __func__, asoc, asoc->pathmtu, asoc->frag_point);
>>>>> }
>>>> The idea was whenever asoc->pathmtu changes, frag_point should
>>>> be updated, but we missed one place in sctp_association_init().
>>>>
>>>> Another issue is after 4-shakehand, the client's asoc->intl_enable
>>>> may be changed from 0 to 1, which means the frag_point should
>>>> also be updated, since [1]:
>>>>
>>>> void sctp_assoc_update_frag_point(struct sctp_association *asoc)
>>>> {
>>>> int frag = sctp_mtu_payload(sctp_sk(asoc->base.sk), asoc->pathmtu,
>>>> sctp_datachk_len(&asoc->stream)); <--- [1]
>>>>
>>>> So one fix for both issues is:
>>>>
>>>> diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
>>>> index 0a78cdf..19d596d 100644
>>>> --- a/net/sctp/stream_interleave.c
>>>> +++ b/net/sctp/stream_interleave.c
>>>> @@ -1327,4 +1327,5 @@ void sctp_stream_interleave_init(struct
>>>> sctp_stream *stream)
>>>> asoc = container_of(stream, struct sctp_association, stream);
>>>> stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
>>>> : &sctp_stream_interleave_0;
>>>> + sctp_assoc_update_frag_point(asoc);
>>>> }
>>>>
>>>>
>>>>> /* Update the association's pmtu and frag_point by going through all the
>>>>> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
>>>>> index ce8087846f05..9f0e64ddbd9c 100644
>>>>> --- a/net/sctp/chunk.c
>>>>> +++ b/net/sctp/chunk.c
>>>>> @@ -190,6 +190,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 (asoc->frag_point < SCTP_FRAG_POINT_MIN) {
>>>>> + pr_warn("%s: asoc:%p->frag_point is less than allowed (%d<%d)",
>>>>> + __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
>>>>> + pr_warn("forcing minimum value to avoid chunking endlessly");
>>>>> + asoc->frag_point = SCTP_FRAG_POINT_MIN;
>>>>> + }
>>>>> max_data = asoc->frag_point;
>>>> This won't happen if we sync frag_point on time like the above.
>>> I know a better fix has been proposed and acked already but I would like to
>>> follow this up a bit. I still think there is some value to the changes in this
>> Any time. :-)
>>
>>> patch. First of all, I am not sure the if statement in sctp_assoc_set_pmtu is
>>> of any practical benefit. I assume its intention is optimization, but I'm
>>> skeptical. Originally it made little sense, since it's not like assigning the
>>> same value would be incorrect or costly. Upon introducing
>> Agree. In SCTP stack usually we are not worried by the effects of a
>> single assignment, especially in control path.
>>
>>> sctp_assoc_update_frag_point I can see the if being of more use since it is
>>> theoretically useless to call this function if MTU is the same, but as it
>>> turns out it might still be useful at what I would estimate to be a negligible
>>> cost. I propose to do away with this if block.
>> Now it is more about consistency. If it is already set to a
>> value and the user sets it again, there should be no behavioral
>> difference. We don't want users calling the same function multiple
>> times for it to "really stick" or so, for example.
>>
>> The if in there, then, now serves to protect it from this.
>>
>>> As for the code in chunk, I'm not too proud of this hacky workaround, but
>>> I still think there should be some way to avoid chunking endlessly and running
>>> OOM if the configuration is wrong (here due to an implementation oversight),
>>> a last-ditch fail-safe of sorts. How do you guys think this could be
>> Good point.
>>
>>> accomplished?
>> Considering that the idea is for sctp_datamsg_from_user to just
>> consume asoc->frag_point, we can add this check right at the end of
>> sctp_assoc_update_frag_point. So that we will always set it to a sane
>> value, no matter what, and warn if it is bogus, for whatever reason.
>> This way it will also warn right when it got set to a bad value.
>>
>> Something like (just a draft):
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 685c7ef11eb4..128a4dd609f3 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -1427,8 +1427,15 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
>>
>> frag = min_t(int, frag, SCTP_MAX_CHUNK_LEN -
>> sctp_datachk_len(&asoc->stream));
>> + frag = SCTP_TRUNC4(frag);
>>
>> - asoc->frag_point = SCTP_TRUNC4(frag);
>> + if (frag < SCTP_FRAG_POINT_MIN) {
>> + pr_warn_ratelimited("%s: asoc:%p frag_point is less than allowed (%d < %d). Forcing to the minimum value.",
>> + __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
>> + frag = SCTP_FRAG_POINT_MIN;
> We don't need a new define here because it should be bounded to the
> same values as user_frag is, like in sctp_setsockopt_maxseg.
>
>> + }
>> +
>> + asoc->frag_point = frag;
>> }
>>
>> void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
> But please scratch this patch. It wouldn't have helped with the
> situation because the lack of calling it is what caused the issue in
> the first place. That said, original patch is the best way I think.
> Notes below still apply, though.
>
> It is tough to find that silver line on how much of sanity checks the
> code should or should not do. My reasoning here is based on the impact
> this issue has. Maybe others have different opinions?
>
>> Note that it should have only 1 pr_warn_ratelimited (no line breaks)
>> and be rate limited because the application could exploit this to
>> trigger endless warnings and having it logged multiple times won't
>> help.
>>
>> Thanks,
>> Marcelo
>>
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.
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.
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);
+}
+
+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/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;
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)) {
+ 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;
+ }
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)
