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/...

[Conrad Meyer]

On Wed, Jan 24, 2018 at 10:05 AM, Warner Losh <imp at 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 — 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 implemented
> 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 uneasy
> 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 >= MUL_NO_OVERFLOW || size >= 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 of
> 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