netdev - Re: [PATCH net] sctp: fix a success return may hide an error

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20160812.211113.1099230839994600349.davem@davemloft.net>
Date:	Fri, 12 Aug 2016 21:11:13 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	lucien.xin@...il.com
Cc:	netdev@...r.kernel.org, linux-sctp@...r.kernel.org,
	marcelo.leitner@...il.com, vyasevich@...il.com,
	daniel@...earbox.net
Subject: Re: [PATCH net] sctp: fix a success return may hide an error

From: Xin Long <lucien.xin@...il.com>
Date: Thu, 11 Aug 2016 20:52:58 +0800

> Now in the end of sctp_outq_flush, sctp calls sctp_packet_transmit
> in a loop. The return of current sctp_packet_transmit always covers
> the prior one's. If the last call of sctp_packet_transmit return a
> success, it may hide the error that returns from the prior call.
> 
> This patch is to fix this by keeping the old error until the new
> error returns from sctp_packet_transmit. Did TAHI test against this
> fix, no regression is found.
> 
> Signed-off-by: Xin Long <lucien.xin@...il.com>

This style of error handling is dangerous.  The first error can be
lost.

For example, if sctp_outq_flush_rtx() earlier in this function returns
an error, it will be lost if any invocation of the function
sctp_packet_transmit() at the end function signals an error.

I think you should always preserve the first error that is recorded
into 'error'.

I also wonder about why sctp_outq_flush_rtx() errors are completely
ignored and don't influence the control flow here in any way.