Date: Fri, 26 Jan 2018 00:40:06 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Conrad Meyer <cem@freebsd.org> Cc: Warner Losh <imp@bsdimp.com>, 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: <20180125220711.D1474@besplex.bde.org> In-Reply-To: <CAG6CVpWvy6o5JgCu=ok_vJjTWgQO2QtRkGhwE2QdHDfcL9-kPQ@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> <CAG6CVpWvy6o5JgCu=ok_vJjTWgQO2QtRkGhwE2QdHDfcL9-kPQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 24 Jan 2018, Conrad Meyer wrote:
> On Wed, Jan 24, 2018 at 10:05 AM, Warner Losh <imp@bsdimp.com> wrote:
> ...
>> 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__.
I already correctly gave this detail and many more. size_t is indeed
the same as u_long on __LP64__. This tends to prevent the compiler
from noticing that the types are are logically different, but I notice.
(clang's error messages for other errors indicate that it knows that
a type typedefed as u_long is logically different from u_long, but it
would be unreasonable for it to warn about mixing them.)
size_t is not the same as u_long on __LP32__, but it has the same
representation. Since it has higher rank, compilers can reasonably
complain about mixing them, like at least gcc does for printing size_t
with %lu format. This helps prevent writing code that will break
(perhaps without the problem being detected at compile time) on arches
where size_t and u_long don't have the same respresentation.
I gave many more details about why the type mismatch doesn't break anything
on any supported arches and what it might break on future arches.
>> 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.
I gave full attention to detail.
> (malloc() should also use size_t.)
That would be just churn.
The newer UMA doesn't even use u_long. It uses plain int for sizes and
counts in all its documented APIs. These are just "int size" and even
"int align" for uma_zcreate() and "int nitems" for uma_zone_set_max().
UMA is fundamentally array-like, with zones consisting of an array of
fairly small items. Since it is a kernel API, it doesn't have to be
consistent with the C design error of using (unsigned) size_t for sizes
and even non-sizes (counts) in malloc() and calloc(). All allocations
are known to be tiny relative to INT32_MAX, at least for individual
elements. (It is almost reasonable to have a single full zone of size
much larger than INT32_MAX on arches with a 64-bit address space. But
UMA depends on callers not asking for this, else it it has many overflows.
Large item sizes are unlikely to occur, and insane nitems for
uma_zone_set_max() is almost self-limiting:
- uma_zone_set_max(zone, -1) calculates a negative page count, then
overflows to large-unsigned on assignment. The result is not much
different from starting with a large positive nitimes
- uma_zone_set_max(zone, large) overflows to a smaller page and item
count than requested. The nitems limit is mostly advisory (for
future allocations), so this might not do anything worse than preventing
future allocations.
>> 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?
Blind use is always unwise. Non-blind use involves looking at the caller
to see if it needs to be more careful. This is only easy for callers that
already have bounds checking.
>> #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.
That wasn't my argument. I said that the correctness of the code depends
on the assumption that size_t has the same representation as u_long and
more, and implicitly that it is too hard to show correctness even if you
assume this and shows inattention to detail if you don't show correctness.
Why not just write code that doesn't make assumptions? Well, this is not
so easy with everything typedefed. It is easier with u_long only. Even
u_long is MD.
I forget if I pointed out the problem with converting the args before
checking them. This is more serious than I relaized before. It gives
the same security hole as blind truncation of the product: suppose that
the args struct *uap has nmemb in an typedefed type that you shouldn't
know. Bad code does:
int *p;
p = mallocarrary(uap->nmemb, sizeof(*p), ..., M_WAITOK);
if (uap->nmemb > 1)
p[uap->nmemb] = 1;
If size_t is uint32_t and typeof(uap->nmemb) is uint64_t, then uap->nmemb
can be 0x100000001 and the bad mallocarray() API truncates this to 1 before
checking for overflow; then the allocation "succeeds" and allocates only 1
element; then the array access is beyond the end of the array even though
the caller appears to check that it is within the array.
malloc(3) and malloc(9)'s API have the same problem -- that they require
the caller to pass the correct args -- but this is not bad since their
reason for existence isn't just to be a wrapper that adds checking.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180125220711.D1474>