From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH net] sctp: fix the issue that a __u16 variable may overflow in sctp_ulpq_renege Date: Mon, 18 Dec 2017 09:33:46 -0500 Message-ID: <20171218143346.GA24106@hmswarspite.think-freely.org> References: <047a7d68a197ff748b48eb8cda4b08fd5b9623fe.1513577245.git.lucien.xin@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: network dev , linux-sctp@vger.kernel.org, davem@davemloft.net, Marcelo Ricardo Leitner To: Xin Long Return-path: Received: from charlotte.tuxdriver.com ([70.61.120.58]:50054 "EHLO smtp.tuxdriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752688AbdLROec (ORCPT ); Mon, 18 Dec 2017 09:34:32 -0500 Content-Disposition: inline In-Reply-To: <047a7d68a197ff748b48eb8cda4b08fd5b9623fe.1513577245.git.lucien.xin@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Dec 18, 2017 at 02:07:25PM +0800, Xin Long wrote: > 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 > --- > 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 a71be33..e36ec5d 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. > -- > 2.1.0 > > Acked-by: Neil Horman