svn commit: r293724 - in head/sys/boot: arm64/libarm64 common efi/boot1 efi/fdt efi/include efi/include/arm64 efi/libefi efi/loader efi/loader/arch/amd64 efi/loader/arch/arm efi/loader/arch/arm64 i...

[Bruce Evans]

On Tue, 12 Jan 2016, Conrad Meyer wrote:

> On Tue, Jan 12, 2016 at 5:32 PM, Ian Lepore <ian at freebsd.org> wrote:
>> Yep, but then I had to do this because ef->off is 64 bits even on 32
>> bit arches, so I got a pointer/int size mismatch warning...
>>
>> Index: common/load_elf.c
>> ===================================================================
>> --- common/load_elf.c   (revision 293796)
>> +++ common/load_elf.c   (working copy)
>> @@ -886,7 +886,7 @@ __elfN(parse_modmetadata)(struct preloaded_file *f
>>         error = __elfN(reloc_ptr)(fp, ef, v, &md, sizeof(md));
>>         if (error == EOPNOTSUPP) {
>>             md.md_cval += ef->off;
>> -           md.md_data = (void *)((uintptr_t)md.md_data + ef->off);
>> +           md.md_data = (void *)(uintptr_t)((uintptr_t)md.md_data +
>> ef->off);
>>         } else if (error != 0)
>>             return (error);
>>  #endif
>>
>> That is just some special kind of ugly.
>
> Yes.  You could maybe do:
>
> md.md_data = (c_caddr_t)md.md_data + (ptrdiff_t)ef->off;

This is much worse.

caddr_t is an old mistake.  It was supposed to be an opaque type
representing a core address, whatever that is.  But opaque types
are even harder to work with.  They might be a pointer of any
type, an integer of any size, floating point, or a struct...
In practice, caddr_t only works for virtual address in flat
memory models.  vm_offset_t or uintptr_t would be the easiest
to work with for that, and void * would be most opaque
(intentionally hard to work with).  But it is actually char *,
and most uses of it are misuses which assume this.

c_caddr_t is a newer mistake.  It is the opaque type caddr_t
peered into to see that it is a pointer and modify that pointer
to pointer to const.

md_data isn't const here, so using c_caddr_t is wronger than usual.

We also need to peer into the type of [c_]caddr_t to know that
it can be assigned to the pointer md_data without a cast.  A
cast would be needed if caddr_t were vm_offset_t or uintptr_t.
Of course, it would be foot-shooting to keep using the poorly
chosen type void * for md_data.  All these addresses and offsets 
should be integers not quite like vm_addr_t and vm_offset_t in
vm (vm_addr_t doesn't exist...).

After further peering, we see that this code shouldn't compile,
since the extra const in c_caddr_t is incompatible with the
plain void * for md_data.

md_cval is const nearby, so it needs a const somewhere in casts.
Since it is plain const char *, using c_caddr_t would be unnatural
for it (but would work because c_caddr_t is const char * obfuscated).

> Instead.  Yes, the ptrdiff_t will truncate uint64_t on 32-bit pointer
> platforms, but the result is truncated regardless when it is stored in
> the md_data pointer.  And the result under modulus is equivalent.

This assumes main things that don't need to be assumed.  After casting
md_data to a pointer instead of an int, ev->off can be added to it
without further casts unless the compiler warns about adding 64-bit
offsets to 32-bit pointers.  But the code already depends on no
warning for the addition in the previous line.  So the cast has no
good effect.

The casts asks for bad effects like truncating the offset to 16 bits
on exotic systems.  ptrdiff_t is only required to be 16 bits and is
not required to actually work even for offsets within a single object.
Using it for general memory offsets asks for undefined behaviour but
only gets it on normal machines for offsets larger than half of the
address space (these overflow).

> (You could even change the type of md_data to c_caddr_t from 'const
> void *' to make it easier to do math on.  Then this could just be:
> md.md_data += (ptrdiff_t)ef->off;)

You mean 'const char *' to give the same type mismatch as c_caddr_t,
or 'char *' to work.  Then remove the cast to ptrdiff_t to give
simple-looking code that works, but still uses a bogus type for
ef->off and a magic type for md_data.  The magic mostly occurs later
when we pass the user pointer md_data directly to the kernel and
cast it to other pointers.

The type mistakes are not large enough for c_caddr_t to be used for any
of the 4 struct members misnamed md_data.  The one here is just the
void * one in <sys/module.h>.  void * is correctly for a general kernel
pointer.

Bruce