Date: Mon, 1 Feb 2016 14:57:37 -0800 From: Conrad Meyer <cem@FreeBSD.org> To: Warner Losh <imp@bsdimp.com> Cc: Mike Belopuhov <mike@belopuhov.com>, "freebsd-arch@freebsd.org" <arch@freebsd.org>, Ryan Stone <rysto32@gmail.com> Subject: Re: OpenBSD mallocarray Message-ID: <CAG6CVpVngXxxSoO=eYTddwwB=eohDtVht7nAbnzmeoqSG3gWwA@mail.gmail.com> In-Reply-To: <CANCZdfprM%2BrkRoEj%2BOaGbMsk3BgyUUDtosMPHCey=Q6aJjWooQ@mail.gmail.com> References: <CAB815ZafpqJoqr1oH8mDJM=0RxLptQJpoJLexw6P6zOi7oSXTQ@mail.gmail.com> <CAG6CVpWbaFOQ1GzE1qmZFodXg_xZafmCc0b1kUh=0%2BFAjLPRvA@mail.gmail.com> <CAFMmRNyNKOgDEY89dVB=dqYDq6XyQo=MQR%2BHPJ2=_0VdDKRvAw@mail.gmail.com> <20160201210256.GA29188@yamori.belopuhov.com> <1EA0ECF5-D7AC-430E-957D-C4D49F9A872B@bsdimp.com> <CAG6CVpUySF%2BbWKW7xvPMxOnYKs8KntSv0pX%2B=X00Qi7=DNithg@mail.gmail.com> <CANCZdfprM%2BrkRoEj%2BOaGbMsk3BgyUUDtosMPHCey=Q6aJjWooQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Feb 1, 2016 at 2:49 PM, Warner Losh <imp@bsdimp.com> wrote: > On Mon, Feb 1, 2016 at 3:19 PM, Conrad Meyer <cem@freebsd.org> wrote: >> On Mon, Feb 1, 2016 at 1:12 PM, Warner Losh <imp@bsdimp.com> wrote: >> > Yea, we don=E2=80=99t want it calling panic. Ever. That turns an overf= low >> > into a DoS. >> >> I disagree. The panic is essentially an assertion that malloc was >> passed valid arguments. We have similar invariants assertions >> throughout the kernel and it is the only sane thing to do with >> overflow + M_WAITOK. M_WAITOK callers today will do something equally >> stupid if they get a NULL result from mallocarray(). > > We only do this for things that can't happen. Exactly. malloc requests that overflow a size_t *cannot* happen. Today they are undefined behavior / at best memory corruption. This is not something we want users to be able to trigger, ever. > If the user can trigger this > panic by passing some bogus, unchecked value that doesn't get checked > until here, that's bad. Agreed. > That's fundamentally different than getting > 'freeing free inode' from ufs. Users can't trigger the latter. Users must not be able to trigger this panic either. > We disagree on this then. This isn't a 'fail safe' this is a 'fail > with denial of system'. that' What is your alternative proposal, in this scenario, that results in any better result than DoS? Heap corruption and code execution is not a better result than DoS, IMO. Keep in mind that if the user controls array size allocation in this scenario, even without the panic they may DoS the system with a huge-but-smaller-than-size_t kernel memory allocation. >> You mean the panic? What fallback behavior would you prefer? If the >> caller requested an overflowing allocation, there really isn't >> anything sane to do besides immediately panic (again, for M_WAITOK). >> Even M_NOWAIT callers may or may not do something sane. > > I'd prefer that we return NULL. Let the caller decide the policy, > not some stupid thing down in the bowls of malloc because > I forgot to include some stupid new flag. If we don't panic explicitly, we panic implicitly for unfixed M_WAITOK users of the interface. I think the explicit panic is better, at least until callers are fixed. > M_NOWAIT callers know to check for NULL, or they have a bug. Yes, but it's a different error. E.g. callers cannot handle EAGAIN like EI= NVAL. > It would be the same for mallocarray: you must check for NULL and > unwind if you get that back. There's no need for the CANFAIL flag > and it's a horrible API. I agree CANFAIL is terrible but come down on the opposite side for the default behavior when given overflowing arguments. Overflowing arguments should never happen. It is a bogus use of the API. Best, Conrad
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpVngXxxSoO=eYTddwwB=eohDtVht7nAbnzmeoqSG3gWwA>