Date: Wed, 24 Jan 2018 14:51:29 -0700 From: Warner Losh <imp@bsdimp.com> To: "Conrad E. Meyer" <cem@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev/... Message-ID: <CANCZdfpBcB8eY80x9=7FQLEfwSD%2BytDtv23_NNa4Pw9W9D7xTg@mail.gmail.com> In-Reply-To: <CAG6CVpXbV9uNNom%2Bc5zzD=ykXM8i_Uyj4krekoyNeHUkC49ekQ@mail.gmail.com> References: <201801211542.w0LFgbsp005980@repo.freebsd.org> <CAG6CVpXxuFyHS11rF=NF6bSSkC2=xnDh=WnbK-aWp4sOomrZ7w@mail.gmail.com> <51ff8aef-5660-7857-e4d5-12cdc77bc071@FreeBSD.org> <20180124182548.X1063@besplex.bde.org> <CANCZdfpL7t_J6xXi8w%2BwtxE%2B4x8EA0AzqzQKno=dUKaJ_NXFrw@mail.gmail.com> <CAG6CVpWv32FjZggRCM3JNWCsMDOSXbVzQfP-dh73fei6Hqr5Mw@mail.gmail.com> <1516817048.42536.182.camel@freebsd.org> <CAG6CVpUpVLe5f=As44TOyePDTy-7LaTKA=rCkk-NjZvZkA5nQg@mail.gmail.com> <CANCZdfqZ5ZiMceh7NZ-YpbgsTW6gqwcxVun0XM_bY2%2BFpustKQ@mail.gmail.com> <2aa48cbd-247a-66cd-b486-02ee77ec2e96@selasky.org> <c7e000e9-6a21-be3e-4037-80e715224f15@FreeBSD.org> <CAG6CVpVo6=e_n7oFWzPeWjq5u%2BG%2BwSV6JvpKF-q4fmxxEJ_MUA@mail.gmail.com> <CANCZdfqxRcsE8MMvnvh-JzHwyb8QZiDpSB9uJmty%2BFQBJqiKcw@mail.gmail.com> <CAG6CVpWTvE%2BFEpn2ScuoZG1je=c0xktnTO4iaO7ByZMLMU99xg@mail.gmail.com> <CANCZdfpQ7chPYYrs%2BO6EwPTeMJhWW=zYeeRnv5hvFda-LO24sQ@mail.gmail.com> <CAG6CVpXbV9uNNom%2Bc5zzD=ykXM8i_Uyj4krekoyNeHUkC49ekQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jan 24, 2018 at 2:40 PM, Conrad Meyer <cem@freebsd.org> wrote: > On Wed, Jan 24, 2018 at 1:13 PM, Warner Losh <imp@bsdimp.com> wrote: > > mallocarray should be the last line of defense, not the only line of > > defense. > > Agreed. > > > most of the time, it's more correct to say > > > > if (WOULD_OVERFLOW(a,b)) > > return EINVAL; > > ptr = mallocarray(a,b...); > > Disagree -- the right check to have outside is much more constrained > than just "will this overflow?" A 10GB allocation request is still > insane on amd64, just for a different reason than on i386. I think > BDE said something along the lines of max 32 kB for most allocations, > and I don't really disagree with that. Picking a specific number for > mallocarray itself (other than overflow) restricts the generality, > though. Well, you're right. there's more context here. You want all these changes to be nops because the upper layers have effectively vetted the arguments. Those that don't need something like the above at the very least. WOULD_OVERFLOW is the least-restrictive version of "is this sane" we have. In the absence of other data, though, it's the last line. > since an error return, assuming it's handled correctly is preferable to a > > panic. > > Agreed. > > > I thought this was more true for NOWAIT than for WAITOK cases, but I've > > realized it's more true always. > > Yeah, I think it's equally true of M_WAITOK and M_NOWAIT. Yea. Upon reflection I've come around to this way of thinking. > And that's why I have such a problem with mallocarray: it's only useful > when > > people are lazy and haven't checked, > > It's useful as a seatbelt. Empirically, people are not perfect at > doing the checking. I can understand that it feels like admitting > laziness to accept that we need a final seatbelt check at all, but I > don't think having the seatbelt hurts. > > > and then it creates a DoS path for > > things that don't check. > > No, again; it doesn't "create" a DoS. Any DoS path was already > present as a heap corruption path. The DoS is strictly an > improvement. Fair enough. We've traded one problem for another. Depending on your threat model one or the other may be preferable. A heap overflow that's not exploitable that the integer overflow may enable might be preferable to a crash if the system uptime is important though. Both are problems that need better filtering. > > We'll change it now and think we're safe, when we > > still have issues, just different issues than before. > > Don't think that, then? We have replaced some classes of heap > corruption with a DoS. That's it; nothing more. I don't think anyone > promoting mallocarray is overrepresenting what it does or claims to > do. My worry is that people are going through and adding it in a sweep based on regexp hits without looking more closely to see if there could possibly be a bigger problem by doing a more in-depth data flow analysis. > It may be a necessary > > change, but it certainly isn't sufficient. > > I don't disagree. > Yea. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpBcB8eY80x9=7FQLEfwSD%2BytDtv23_NNa4Pw9W9D7xTg>