| 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: <50C60496.5040800@ericsson.com>
Date: Mon, 10 Dec 2012 10:49:42 -0500
From: Jon Maloy <jon.maloy@...csson.com>
To: Ying Xue <ying.xue@...driver.com>
CC: <nhorman@...driver.com>, <Paul.Gortmaker@...driver.com>,
<erik.hugne@...csson.com>, <netdev@...r.kernel.org>,
<tipc-discussion@...ts.sourceforge.net>
Subject: Re: [PATCH net-next v3] tipc: sk_recv_queue size check only for connectionless
sockets
On 12/10/2012 05:13 AM, Jon Maloy wrote:
> On 12/10/2012 04:23 AM, Ying Xue wrote:
>> The sk_receive_queue limit control is currently performed for all
>> arriving messages, disregarding socket and message type. But for
>> connectionless sockets this check is redundant, since the protocol
>> flow already makes queue overflow impossible.
>>
>> We move the sk_receive_queue limit control so that it's only performed
>> for connectionless sockets, i.e. SOCK_RDM and SOCK_DGRAM type sockets.
>>
>> However, as Neil Horman specified, we cannot simply force the socket
>> receive queue limit against connectionless sockets as it may create a
>> DoS vulnerability. For example, if a sender floods a receiver with
>> messages containing an invalid set of message importance bits or
>> CRITICAL importance, we will queue messages indefinitely.
>>
>> To avoid DoS attack, socket receive queue will be marked as overflow
>> if we receive messages with invalid message importances, meanwhile,
>> we also set one new threshold for CRITICAL importance messages.
>>
>> Signed-off-by: Ying Xue <ying.xue@...driver.com>
>> Signed-off-by: Jon Maloy <jon.maloy@...csson.com>
>> Cc: Neil Horman <nhorman@...driver.com>
>> Signed-off-by: Paul Gortmaker <paul.gortmaker@...driver.com>
>> ---
>> v3 changes:
>> - set new threshold for CRITICAL message
>> - defined an importance factor table to avoid multiplication and
>> division operations in rx_queue_full().
>> - changed return value of rx_queue_full() from integer to boolean.
>>
>> net/tipc/socket.c | 44 +++++++++++++++++++-------------------------
>> 1 files changed, 19 insertions(+), 25 deletions(-)
>>
>> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
>> index 9b4e483..a18a757 100644
>> --- a/net/tipc/socket.c
>> +++ b/net/tipc/socket.c
>> @@ -43,7 +43,7 @@
>> #define SS_LISTENING -1 /* socket is listening */
>> #define SS_READY -2 /* socket is connectionless */
>>
>> -#define OVERLOAD_LIMIT_BASE 10000
>> +#define OVERLOAD_LIMIT_BASE 5000
>> #define CONN_TIMEOUT_DEFAULT 8000 /* default connect timeout = 8s */
>>
>> struct tipc_sock {
>> @@ -73,6 +73,13 @@ static struct proto tipc_proto;
>>
>> static int sockets_enabled;
>>
>> +static const u32 msg_importance_factor[] = {
>> + OVERLOAD_LIMIT_BASE, /* TIPC_LOW_IMPORTANCE limit */
>> + OVERLOAD_LIMIT_BASE * 2, /* TIPC_MEDIUM_IMPORTANCE limit */
>> + OVERLOAD_LIMIT_BASE * 100, /* TIPC_HIGH_IMPORTANCE limit */
>> + OVERLOAD_LIMIT_BASE * 200 /* TIPC_CRITICAL_IMPORTANCE limit */
>> + };
>> +
>> /*
>> * Revised TIPC socket locking policy:
>> *
>> @@ -1158,28 +1165,17 @@ static void tipc_data_ready(struct sock *sk, int len)
>> * rx_queue_full - determine if receive queue can accept another message
>> * @msg: message to be added to queue
>> * @queue_size: current size of queue
>> - * @base: nominal maximum size of queue
>> *
>> - * Returns 1 if queue is unable to accept message, 0 otherwise
>> + * Returns true if queue is unable to accept message, false otherwise
>> */
>> -static int rx_queue_full(struct tipc_msg *msg, u32 queue_size, u32 base)
>> +static bool rx_queue_full(struct tipc_msg *msg, u32 queue_size)
>> {
>> - u32 threshold;
>> u32 imp = msg_importance(msg);
>>
>> - if (imp == TIPC_LOW_IMPORTANCE)
>> - threshold = base;
>> - else if (imp == TIPC_MEDIUM_IMPORTANCE)
>> - threshold = base * 2;
>> - else if (imp == TIPC_HIGH_IMPORTANCE)
>> - threshold = base * 100;
>> - else
>> - return 0;
>> + if (unlikely(imp > TIPC_CRITICAL_IMPORTANCE))
>> + return true;
>
> This test is not necessary. Such messages have already been filtered out
> in tipc_recv_msg() at link level.
> The test msg_isdata(), which determines if a message should be sent up to
> the port/socket level, is also an implicit test that
> importance < TIPC_CRITICAL_IMPORTANCE.
(importance <= TIPC_CRITICAL_IMPORTANCE), of course.
This is an effect of the co-location of the user and importance fields in the
TIPC header. I.e., the importance is in reality coded into the value of
the user field.
To clarify (and improve) my previous suggestion, what I had in mind
was something like this:
recv_q_len = skb_queue_len(&sk->sk_receive_queue);
- if (unlikely(recv_q_len >= (OVERLOAD_LIMIT_BASE / 2))) {
- if (rx_queue_full(msg, recv_q_len,
- OVERLOAD_LIMIT_BASE / 2))
- return TIPC_ERR_OVERLOAD;
- }
+ imp = msg_importance(msg);
+ if (unlikely(recv_q_len > (OVERLOAD_LIMIT_BASE << (imp << 1))))
+ return TIPC_ERR_OVERLOAD;
} else {
if (msg_mcast(msg))
return TIPC_ERR_NO_PORT;
No need for any separate function at all, and guaranteed inline.
This will probably translate to one single shift-instruction extra,
and no memor access, since the compiler merge (imp << 1) with the
shift operation used to read imp from the header.
The limits for message rejection become the following,
given an OVERLOAD_LIMIT_BASE of 10000:
TIP_LOW_IMPORTANCE: 10000 (previously 10000)
TIPC_MEDIUM_IMPORTANCE 40000 (previously 20000)
TIPC_MEDIUM_IMPORTANCE 1600000 (previously 1000000)
TIPC_CRITICAL_IMPORTANCE 25600000 (previously no limit)
///jon
>
>
>>
>> - if (msg_connected(msg))
>> - threshold *= 4;
>> -
>> - return queue_size >= threshold;
>> + return queue_size >= msg_importance_factor[imp];
>
> Ok. Less optimal than my suggestion, but also lower risk until we know
> the consequences of changing the multiplication factors.
>
>> }
>>
>> /**
>> @@ -1275,7 +1271,6 @@ static u32 filter_rcv(struct sock *sk, struct sk_buff *buf)
>> {
>> struct socket *sock = sk->sk_socket;
>> struct tipc_msg *msg = buf_msg(buf);
>> - u32 recv_q_len;
>> u32 res = TIPC_OK;
>>
>> /* Reject message if it is wrong sort of message for socket */
>> @@ -1285,19 +1280,18 @@ static u32 filter_rcv(struct sock *sk, struct sk_buff *buf)
>> if (sock->state == SS_READY) {
>> if (msg_connected(msg))
>> return TIPC_ERR_NO_PORT;
>> + /* Reject SOCK_DGRAM and SOCK_RDM message if there isn't room
>> + * to queue it
>> + */
>> + if (unlikely(rx_queue_full(msg,
>> + skb_queue_len(&sk->sk_receive_queue))))
>> + return TIPC_ERR_OVERLOAD;
>> } else {
>> res = filter_connect(tipc_sk(sk), &buf);
>> if (res != TIPC_OK || buf == NULL)
>> return res;
>> }
>>
>> - /* Reject message if there isn't room to queue it */
>> - recv_q_len = skb_queue_len(&sk->sk_receive_queue);
>> - if (unlikely(recv_q_len >= (OVERLOAD_LIMIT_BASE / 2))) {
>> - if (rx_queue_full(msg, recv_q_len, OVERLOAD_LIMIT_BASE / 2))
>> - return TIPC_ERR_OVERLOAD;
>> - }
>> -
>> /* Enqueue message (finally!) */
>> TIPC_SKB_CB(buf)->handle = 0;
>> __skb_queue_tail(&sk->sk_receive_queue, buf);
>>
>
> --
> 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
>
--
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