netdev - Re: [PATCH 2/2 net] sctp: fix an error code in sctp_sf_eat

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: <7899ff13-ab06-4970-a306-85b218486571@kadam.mountain>
Date: Fri, 9 Jun 2023 19:41:31 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Xin Long <lucien.xin@...il.com>
Cc: Vlad Yasevich <vladislav.yasevich@...com>,
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-sctp@...r.kernel.org, netdev@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 2/2 net] sctp: fix an error code in sctp_sf_eat_auth()

On Fri, Jun 09, 2023 at 11:13:03AM -0400, Xin Long wrote:
> This one looks good to me.
> 
> But for the patch 1/2 (somehow it doesn't show up in my mailbox):
> 
>   default:
>   pr_err("impossible disposition %d in state %d, event_type %d, event_id %d\n",
>         status, state, event_type, subtype.chunk);
> - BUG();
> + error = status;
> + if (error >= 0)
> + error = -EINVAL;
> + WARN_ON_ONCE(1);
> 
> I think from the sctp_do_sm() perspective, it expects the state_fn
> status only from
> enum sctp_disposition. It is a BUG to receive any other values and
> must be fixed,
> as you did in 2/2. It does the same thing as other functions in SCTP code, like
> sctp_sf_eat_data_*(), sctp_retransmit() etc.

It is a bug, sure.  And after my patch is applied it will still trigger
a stack trace.  But we should only call the actual BUG() function
in order to prevent filesystem corruption or a privilege escalation or
something along those lines.

Calling BUG() makes the system unusable so it makes bugs harder to
debug.  This is even mentioned in checkpatch.pl "Do not crash the kernel
unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery
code (if feasible) instead of BUG() or variants".

regards,
dan carpenter