From owner-svn-src-all@freebsd.org Wed Jan 24 21:51:31 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 496A1EC8740 for ; Wed, 24 Jan 2018 21:51:31 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-wm0-x22c.google.com (mail-wm0-x22c.google.com [IPv6:2a00:1450:400c:c09::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 94FFF7A6A6 for ; Wed, 24 Jan 2018 21:51:30 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-wm0-x22c.google.com with SMTP id v123so11352092wmd.5 for ; Wed, 24 Jan 2018 13:51:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=Uf/eQMifOyCeWtl5qOyeuGEHyfSUxYdHyfPCSl48vDg=; b=ZLD5eGbvSahS5wiohOFL1V3RElzu9t0UMTzhhrlyVoEB1UPYnYu5Uw07pVb6GgDDD6 e6G17MMI0r9qv7w75MsQTMZVvnK5/9xX7tzMSUQVOfQtI+Dit+tw2D3XEvurHcMdvFai p1kIFlk7h4Pes1r/hS7qBxfRR9yUDYgAOc7nVD3NdNfSvfV9ULqT9t60gOjYTidKrvJM A5D4AJo9NA+51x/klE5r5KF7f0/2YE3WeqvH5Naiq9pifgVmLzKc/TPaG3+pnp596hov ZY5BMVu8k6ifhm6ck4i737Rr/SQOWQsPP2XEOqzZMnv8jfdCNfAuluCYcaRTtWHpCnDa NnWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=Uf/eQMifOyCeWtl5qOyeuGEHyfSUxYdHyfPCSl48vDg=; b=pKllQJwr2SaKWe4iIFzGXELrZnq9ojBF+ac7CvARm4JuS/zin/vAnjCa4CtsBZ0erL ayzS0bN60MkX3e7qcWuZy0eMl6hRkwvd94NHm/Z6WXDUIGD1m7wsVr3g+nDteM75ONhk IMh8IfmeU3vvpmgAOsPzbYVKrlvOStFYmVoO97cdzRMnC6T5xyBvxAMz4Hfxarwmd3P7 TSGQmAjSMPB7rPWSG06BJMPqK/Cx0ScjNmPpJoUH2njpqIPuq0glHYlh7jiIBcmP242l h6mBMGT11c+3K7jXzHk/xgnyOfuOiIxa9dcxhRn9xCFt+KTptTvEUeXEZYEOFMxNM7xD KFWg== X-Gm-Message-State: AKwxyteFppXMqN2lzKS8StdnlKo4pA9VGtGbXJKjdH8hgfuk5F2wFvdU /sSwf7VvPXbdw0DOJo4O9w2KFiBkCyaLjvYh3hqzpg== X-Google-Smtp-Source: AH8x226VlUss8kWpTnitxqJT1JB52iIOVBrPuHd3jiT7eEl8zD25QzLWIM+1hD7ND5oXNtXOIKpzBDGQ9Q2TPSDgYeU= X-Received: by 10.80.170.24 with SMTP id o24mr26154629edc.258.1516830689433; Wed, 24 Jan 2018 13:51:29 -0800 (PST) MIME-Version: 1.0 Sender: wlosh@bsdimp.com Received: by 10.80.133.195 with HTTP; Wed, 24 Jan 2018 13:51:29 -0800 (PST) X-Originating-IP: [2603:300b:6:5100:1052:acc7:f9de:2b6d] In-Reply-To: References: <201801211542.w0LFgbsp005980@repo.freebsd.org> <51ff8aef-5660-7857-e4d5-12cdc77bc071@FreeBSD.org> <20180124182548.X1063@besplex.bde.org> <1516817048.42536.182.camel@freebsd.org> <2aa48cbd-247a-66cd-b486-02ee77ec2e96@selasky.org> From: Warner Losh Date: Wed, 24 Jan 2018 14:51:29 -0700 X-Google-Sender-Auth: u1yzUfuZg94m5EHnAoOxM6JEZ1o Message-ID: 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/... To: "Conrad E. Meyer" Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.25 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Jan 2018 21:51:31 -0000 On Wed, Jan 24, 2018 at 2:40 PM, Conrad Meyer wrote: > On Wed, Jan 24, 2018 at 1:13 PM, Warner Losh 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