Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Jon Maloy <jon.maloy@ericsson.com>
To:     Wenwen Wang <wang6495@umn.edu>
CC:     Kangjie Lu <kjlu@umn.edu>, Ying Xue <ying.xue@windriver.com>,
        "David S. Miller" <davem@davemloft.net>,
        "open list:TIPC NETWORK LAYER" <netdev@vger.kernel.org>,
        "open list:TIPC NETWORK LAYER" 
        <tipc-discussion@lists.sourceforge.net>,
        open list <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] tipc: fix a potential missing-check bug
Thread-Topic: [PATCH] tipc: fix a potential missing-check bug
Thread-Index: AQHT4QSepsg7SNyeDE2wQK/XNU8Hd6QavkyQ
Date:   Tue, 1 May 2018 12:01:22 +0000
Message-ID: <BN6PR15MB1553F08DDF8D0F1356AA72EA9A810@BN6PR15MB1553.namprd15.prod.outlook.com>
References: <1525148761-11091-1-git-send-email-wang6495@umn.edu>
In-Reply-To: <1525148761-11091-1-git-send-email-wang6495@umn.edu>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: ericsson.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b1f42af4-b6f0-4a03-1b22-08d5af5b422f
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 May 2018 12:01:22.8299
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 92e84ceb-fbfd-47ab-be52-080c6b87953f
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR15MB1170
X-OriginatorOrg: ericsson.com
X-Brightmail-Tracker: H4sIAAAAAAAAA01Sa0iTURjufJf5ORqclpcXS8HRj/Cu9GOJinZdCBJUmKOwpR9q3jeVlCAz
        MzEtkVlpK6eluDBTNzU1BDXFhmZoeSlJSldOC23iJUvN7VvQv+f2Pu954TCksJ52YuKS0lh5
        kixBxONTZWdbXTwdwmelPjlv+WLVUC4l7n80aCMeaVfxxH1qB7Fu6JJYr/9Ei6fHxshgG4lO
        M0FIOtRLhOR9ZY6NZKnJRfJc10ycpKX8gGg2IS6DlXsHXeDHKucmeSkPHC/P1hjobLQhLEC2
        DOAD0PNTRxQgPiPErxDczl224YgWQevn61ayjKCpuB9x5DEBRo2WNhMKmwgYGJiwxkoI2Ghc
        JDnyBUHzsppnXsPD+8F4Nx+ZsR0WQd5ovmWcxCoC1lXztNnYjQNgoW+K4EKBoGys39aZbewH
        64NSs0zhfXCnqNYSF+BzMNo1ZekU4mBoGChE5rgtDoH7Bg+zjLADrOrrLI0kdoQPMxUEdzWG
        Jy+HSA7bg3F6k+awKww1VFl1ZxiuuGU5GbCOAMNIr3XYExZLS0nOaEHQudLM4ww3mChutjbF
        Q8t4Oc2FChBsLd+0VlWTkP+xhOJSe6HfVEQXI9/y/57IYQ9Qd5h4HHaHmsp5stxy9S54XTZD
        qRH1FNkrWMXFxBg/Py9WHhelUCQneSWxaU1o+xd16X77vEDGbyHdCDNItFPQcGRWKqRlGYrM
        xG4EDCmyE2jyv0qFgmhZZhYrT46Upyewim60h6FEjgJxmFYqxDGyNDaeZVNY+T+XYGydspG3
        p2Zae9y7LVU1UfhsnDqm9DvvpDEc7TrofyL0zbV2/9XQ9JbYtULRGX11aGrt5q/ww7ecTfVh
        c1Rej7s0MGAy5aEgfkX77oomPIxfFWc7m6Ks61w4pbzx409UxOlIg0Q5nLe2QxAws9bb1phu
        Clv/ntPS55p9aCvr3tWgiPIijYhSxMp83Ui5QvYX7iN1/kEDAAA=
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Wenwen Wang [mailto:wang6495@umn.edu]
> Sent: Tuesday, May 01, 2018 00:26
> To: Wenwen Wang <wang6495@umn.edu>
> Cc: Kangjie Lu <kjlu@umn.edu>; Jon Maloy <jon.maloy@ericsson.com>; Ying
> Xue <ying.xue@windriver.com>; David S. Miller <davem@davemloft.net>;
> open list:TIPC NETWORK LAYER <netdev@vger.kernel.org>; open list:TIPC
> NETWORK LAYER <tipc-discussion@lists.sourceforge.net>; open list <linux-
> kernel@vger.kernel.org>
> Subject: [PATCH] tipc: fix a potential missing-check bug
>=20
> In tipc_link_xmit(), the member field "len" of l->backlog[imp] must be le=
ss
> than the member field "limit" of l->backlog[imp] when imp is equal to
> TIPC_SYSTEM_IMPORTANCE. Otherwise, an error code, i.e., -ENOBUFS, is
> returned. This is enforced by the security check. However, at the end of
> tipc_link_xmit(), the length of "list" is added to l->backlog[imp].len wi=
thout
> any further check. This can potentially cause unexpected values for
> l->backlog[imp].len. If imp is equal to TIPC_SYSTEM_IMPORTANCE and the
> original value of l->backlog[imp].len is less than l->backlog[imp].limit,=
 after
> this addition, l->backlog[imp] could be larger than
> l->backlog[imp].limit.=20

It can, but only once. That is the intention with allowing oversubscription=
. This is expected and permitted.
At next sending attempt, if the send queue has not been reduced in the mean=
time, the link will be reset, as intended.

> That means the security check can potentially be
> bypassed,  especially when an adversary can control the length of "list".

The length of 'list' is entirely controlled by TIPC itself, either by the s=
ocket layer (where length  always is 1 for this type of messages) or
 name_dist, In the latter case the length is also 1, except at first link s=
etup, when there guaranteed is no congestion anyway.

I appreciate your interest, but this patch is not needed.

BR
///jon

>=20
> This patch performs such a check after the modification to
> l->backlog[imp].len (if imp is TIPC_SYSTEM_IMPORTANCE) to avoid such
> security issues. An error code will be returned if an unexpected value of
> l->backlog[imp].len is generated.
>=20
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  net/tipc/link.c | 5 +++++
>  1 file changed, 5 insertions(+)
>=20
> diff --git a/net/tipc/link.c b/net/tipc/link.c index 695acb7..62972fa 100=
644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -948,6 +948,11 @@ int tipc_link_xmit(struct tipc_link *l, struct
> sk_buff_head *list,
>  			continue;
>  		}
>  		l->backlog[imp].len +=3D skb_queue_len(list);
> +		if (imp =3D=3D TIPC_SYSTEM_IMPORTANCE &&
> +		    l->backlog[imp].len >=3D l->backlog[imp].limit) {
> +			pr_warn("%s<%s>, link overflow", link_rst_msg, l-
> >name);
> +			return -ENOBUFS;
> +		}
>  		skb_queue_splice_tail_init(list, backlogq);
>  	}
>  	l->snd_nxt =3D seqno;
> --
> 2.7.4