On Wed, Jan 24, 2018 at 10:05 AM, Warner Losh <i...@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
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
