From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: David Laight To: 'Sasha Levin' , "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" CC: Xin Long , "David S . Miller" Subject: RE: [PATCH AUTOSEL for 4.14 019/110] sctp: fix the issue that a __u16 variable may overflow in sctp_ulpq_renege Date: Mon, 5 Feb 2018 11:35:56 +0000 Message-ID: <4eedae3f4cb14e178c073c183ef134c6@AcuMS.aculab.com> References: <20180203180015.29073-1-alexander.levin@microsoft.com> <20180203180015.29073-19-alexander.levin@microsoft.com> In-Reply-To: <20180203180015.29073-19-alexander.levin@microsoft.com> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: From: Sasha Levin > Sent: 03 February 2018 18:01 > [ Upstream commit 5c468674d17056148da06218d4da5d04baf22eac ] > > Now when reneging events in sctp_ulpq_renege(), the variable freed > could be increased by a __u16 value twice while freed is of __u16 > type. It means freed may overflow at the second addition. > > This patch is to fix it by using __u32 type for 'freed', while at > it, also to remove 'if (chunk)' check, as all renege commands are > generated in sctp_eat_data and it can't be NULL. > > Reported-by: Marcelo Ricardo Leitner > Signed-off-by: Xin Long > Acked-by: Neil Horman > Signed-off-by: David S. Miller > Signed-off-by: Sasha Levin > --- > net/sctp/ulpqueue.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c > index a71be33f3afe..e36ec5dd64c6 100644 > --- a/net/sctp/ulpqueue.c > +++ b/net/sctp/ulpqueue.c > @@ -1084,29 +1084,21 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq, > void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk, > gfp_t gfp) > { > - struct sctp_association *asoc; > - __u16 needed, freed; > - > - asoc = ulpq->asoc; > + struct sctp_association *asoc = ulpq->asoc; > + __u32 freed = 0; > + __u16 needed; > > - if (chunk) { > - needed = ntohs(chunk->chunk_hdr->length); > - needed -= sizeof(struct sctp_data_chunk); > - } else > - needed = SCTP_DEFAULT_MAXWINDOW; > - > - freed = 0; > + needed = ntohs(chunk->chunk_hdr->length) - > + sizeof(struct sctp_data_chunk); > > if (skb_queue_empty(&asoc->base.sk->sk_receive_queue)) { > freed = sctp_ulpq_renege_order(ulpq, needed); > - if (freed < needed) { > + if (freed < needed) > freed += sctp_ulpq_renege_frags(ulpq, needed - freed); > - } > } > /* If able to free enough room, accept this chunk. */ > - if (chunk && (freed >= needed)) { > - int retval; > - retval = sctp_ulpq_tail_data(ulpq, chunk, gfp); > + if (freed >= needed) { > + int retval = sctp_ulpq_tail_data(ulpq, chunk, gfp); > /* > * Enter partial delivery if chunk has not been > * delivered; otherwise, drain the reassembly queue. Hmmm... ISTM that all the maths should be done using 'unsigned int' to avoid horrid masking operations on many cpus.... David