Re: [PATCH 4.4 31/36] sctp: verify size of a new chunk in _sctp_make_chunk()

[Ben Hutchings]

On Tue, 2018-03-13 at 10:56 +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 13, 2018 at 12:46:58AM +0000, Ben Hutchings wrote:
> > On Fri, 2018-03-09 at 16:18 -0800, Greg Kroah-Hartman wrote:
> > > 4.4-stable review patch.ÂÂIf anyone has any objections, please
> > > let me know.
> > > 
> > > ------------------
> > > 
> > > From: Alexey Kodanev <alexey.kodanev@xxxxxxxxxx>
> > > 
> > > 
> > > [ Upstream commit 07f2c7ab6f8d0a7e7c5764c4e6cc9c52951b9d9c ]
> > > 
> > > When SCTP makes INIT or INIT_ACK packet the total chunk length
> > > can exceed SCTP_MAX_CHUNK_LEN which leads to kernel panic when
> > > transmitting these packets, e.g. the crash on sending INIT_ACK:
> > > 
> > > [ÂÂ597.804948] skbuff: skb_over_panic: text:00000000ffae06e4
> > > len:120168
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂput:120156 head:000000007aa47635
> > > data:00000000d991c2de
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtail:0x1d640 end:0xfec0 dev:<NULL>
> > > ...
> > > [ÂÂ597.976970] ------------[ cut here ]------------
> > > [ÂÂ598.033408] kernel BUG at net/core/skbuff.c:104!
> > > [ÂÂ600.314841] Call Trace:
> > > [ÂÂ600.345829]ÂÂ<IRQ>
> > > [ÂÂ600.371639]ÂÂ? sctp_packet_transmit+0x2095/0x26d0 [sctp]
> > > [ÂÂ600.436934]ÂÂskb_put+0x16c/0x200
> > > [ÂÂ600.477295]ÂÂsctp_packet_transmit+0x2095/0x26d0 [sctp]
> > > [ÂÂ600.540630]ÂÂ? sctp_packet_config+0x890/0x890 [sctp]
> > > [ÂÂ600.601781]ÂÂ? __sctp_packet_append_chunk+0x3b4/0xd00 [sctp]
> > > [ÂÂ600.671356]ÂÂ? sctp_cmp_addr_exact+0x3f/0x90 [sctp]
> > > [ÂÂ600.731482]ÂÂsctp_outq_flush+0x663/0x30d0 [sctp]
> > > [ÂÂ600.788565]ÂÂ? sctp_make_init+0xbf0/0xbf0 [sctp]
> > > [ÂÂ600.845555]ÂÂ? sctp_check_transmitted+0x18f0/0x18f0 [sctp]
> > > [ÂÂ600.912945]ÂÂ? sctp_outq_tail+0x631/0x9d0 [sctp]
> > > [ÂÂ600.969936]ÂÂsctp_cmd_interpreter.isra.22+0x3be1/0x5cb0 [sctp]
> > > [ÂÂ601.041593]ÂÂ? sctp_sf_do_5_1B_init+0x85f/0xc30 [sctp]
> > > [ÂÂ601.104837]ÂÂ? sctp_generate_t1_cookie_event+0x20/0x20 [sctp]
> > > [ÂÂ601.175436]ÂÂ? sctp_eat_data+0x1710/0x1710 [sctp]
> > > [ÂÂ601.233575]ÂÂsctp_do_sm+0x182/0x560 [sctp]
> > > [ÂÂ601.284328]ÂÂ? sctp_has_association+0x70/0x70 [sctp]
> > > [ÂÂ601.345586]ÂÂ? sctp_rcv+0xef4/0x32f0 [sctp]
> > > [ÂÂ601.397478]ÂÂ? sctp6_rcv+0xa/0x20 [sctp]
> > > ...
> > > 
> > > Here the chunk size for INIT_ACK packet becomes too big, mostly
> > > because of the state cookie (INIT packet has large size with
> > > many address parameters), plus additional server parameters.
> > > 
> > > Later this chunk causes the panic in skb_put_data():
> > > 
> > > Â skb_packet_transmit()
> > > ÂÂÂÂÂÂsctp_packet_pack()
> > > ÂÂÂÂÂÂÂÂÂÂskb_put_data(nskb, chunk->skb->data, chunk->skb->len);
> > > 
> > > 'nskb' (head skb) was previously allocated with packet->size
> > > from u16 'chunk->chunk_hdr->length'.
> > > 
> > > As suggested by Marcelo we should check the chunk's length in
> > > _sctp_make_chunk() before trying to allocate skb for it and
> > > discard a chunk if its size bigger than SCTP_MAX_CHUNK_LEN.
> > > 
> > > > Signed-off-by: Alexey Kodanev <alexey.kodanev@xxxxxxxxxx>
> > > > Acked-by: Marcelo Ricardo Leitner <marcelo.leinter@xxxxxxxxx>
> > > > Acked-by: Neil Horman <nhorman@xxxxxxxxxxxxx>
> > > > Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > 
> > > ---
> > > Ânet/sctp/sm_make_chunk.c |ÂÂÂÂ8 ++++++--
> > > Â1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > --- a/net/sctp/sm_make_chunk.c
> > > +++ b/net/sctp/sm_make_chunk.c
> > > @@ -1367,10 +1367,14 @@ static struct sctp_chunk *_sctp_make_chu
> > > Â	sctp_chunkhdr_t *chunk_hdr;
> > > Â	struct sk_buff *skb;
> > > Â	struct sock *sk;
> > > +	int chunklen;
> > > +
> > > +	chunklen = sizeof(*chunk_hdr) + paylen;
> > 
> > I think this length still needs to be rounded up (with WORD_ROUND
> > here,
> > instead of SCTP_PAD4 upstream).
> 
> Ah, good point, how's this patch:

LGTM.

Ben.

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 4ca31e052dd8..509e9426a056 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1369,7 +1369,7 @@ static struct sctp_chunk
> *_sctp_make_chunk(const struct sctp_association *asoc,
> Â	struct sock *sk;
> Â	int chunklen;
> Â
> -	chunklen = sizeof(*chunk_hdr) + paylen;
> +	chunklen = WORD_ROUND(sizeof(*chunk_hdr) + paylen);
> Â	if (chunklen > SCTP_MAX_CHUNK_LEN)
> Â		goto nodata;
> Â
> 
-- 
Ben Hutchings
Software Developer, Codethink Ltd.