Date: Wed, 24 Jan 2018 10:30:03 -0800 From: Conrad Meyer <cem@freebsd.org> To: Warner Losh <imp@bsdimp.com> 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: <CAG6CVpWvy6o5JgCu=ok_vJjTWgQO2QtRkGhwE2QdHDfcL9-kPQ@mail.gmail.com> In-Reply-To: <CANCZdfoD8bXnn6=nmX3UGkwKnDdCAvXvwMMyh=pm1Tiz15wvtQ@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> <CANCZdfoD8bXnn6=nmX3UGkwKnDdCAvXvwMMyh=pm1Tiz15wvtQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jan 24, 2018 at 10:05 AM, Warner Losh <imp@bsdimp.com> wrote:
> It changes the fundamental 'can't fail' allocations into 'maybe panic'
> allocations, which was my big objection.
No, it doesn't make any such change. The only calls that will panic
now would have "succeeded" before but returned a smaller size than the
naive caller thought they were asking for. This allows an attacker to
dereference beyond the ends of the actually allocated region.
>> Your description of two years ago is inaccurate =E2=80=94 you thought it=
was a
>> bad idea, and were the most vocal on the mailing list about it, but
>> that viewpoint was not universally shared. In a pure headcount vote I
>> think you were even outvoted, but as the initiative was headed by a
>> non-committer, it sputtered out.
>
>
> I don't recall that happening. But regardless, mallocarray, as implemente=
d
> today, is useless.
Search your email inbox for the mallocarray thread from Feb 2016. I
don't think it's useless.
> Let's start with his point about u_long vs size_t causing problems:
>
> void *malloc(unsigned long size, struct malloc_type *type, int flags)
> vs
> void *mallocarray(size_t nmemb, size_t size, struct malloc_type *type,
>
> Since size_t is long long on i386, for example,
It is? Since when? __size_t is uint32_t on !__LP64__.
> this can result in
> undetected overflows. Such inattention to detail makes me extremely uneas=
y
> about the rest of the code.
A similar snarky comment can be made here about inattention to detail
when making criticisms. If __LP64__ is true, long long is the same
size as long anyway.
(malloc() should also use size_t.)
> It's an important function, but it's so poorly implement we should try
> again. It is not safe nor wise to use it blindly, which was bde's larger
> point.
Citation needed?
> #define MUL_NO_OVERFLOW (1UL << (sizeof(size_t) * 8 / 2))
> static inline bool
> WOULD_OVERFLOW(size_t nmemb, size_t size)
> {
>
> return ((nmemb >=3D MUL_NO_OVERFLOW || size >=3D MUL_NO_OVERFLOW)=
&&
> nmemb > 0 && __SIZE_T_MAX / nmemb < size);
> }
>
> So if I pass in 1GB and 10, this will tell me it won't overflow because o=
f
> size_t can handle 10GB, but u_long can't.
This whole argument hinges upon incorrect assumption that size_t is
larger than u_long.
> ... (deleted bogus argument)
> Many places that use mallocarray likely should use WOULD_OVERFLOW instead=
to
> do proper error handling, assuming it is fixed to match the actual malloc
> interface. This is especially true in the NO_WAIT cases.
I disagree. They should be doing a much more restrictive check
instead. WOULD_OVERFLOW is really the lowest bar, a seatbelt. I
agree with Bruce that most callers should instead be checking for
sizes in the dozens of kB range. Both checks are useful.
Best,
Conrad
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpWvy6o5JgCu=ok_vJjTWgQO2QtRkGhwE2QdHDfcL9-kPQ>