Page MenuHomeFreeBSD

xen: Fix warning by adding KERNBASE to vm_paddr_t before casting
ClosedPublic

Authored by dim on Aug 29 2021, 2:04 PM.

Details

Summary

Clang 13 produces the following warning for hammer_time_xen():

sys/x86/xen/pv.c:183:19: error: the pointer incremented by -2147483648 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements) [-Werror,-Warray-bounds]
                    (vm_paddr_t)start_info->modlist_paddr + KERNBASE;
                                ^                           ~~~~~~~~
sys/xen/interface/arch-x86/hvm/start_info.h:131:5: note: array 'modlist_paddr' declared here
    uint64_t modlist_paddr;     /* Physical address of an array of           */
    ^

This is because the expression first casts start_info->modlist_paddr
to struct hvm_modlist_entry * (via vmpaddr_t), and *then* adds
KERNBASE, which is then interpreted as KERNBASE * sizeof(struct
hvm_modlist_entry).

Instead, parenthesize the addition to get the intended result, and cast
it to struct hvm_modlist_entry * afterwards.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dim requested review of this revision.Aug 29 2021, 2:04 PM

I think you can also remove the cast to vm_paddr_t while there, the field is declared as uint64_t.

This revision is now accepted and ready to land.Aug 29 2021, 2:15 PM

I think you can also remove the cast to vm_paddr_t while there, the field is declared as uint64_t.

Hm, won't that give problems on i386? I'll see what a i386 build does.

In D31711#715619, @dim wrote:

I think you can also remove the cast to vm_paddr_t while there, the field is declared as uint64_t.

Hm, won't that give problems on i386? I'll see what a i386 build does.

That code is only built for amd64, I don't have plans to use it in i386.

On second thought it might have been easier to just cast to void * and forget about the parentheses.

No, then you would be doing arithmetic on a void pointer, which isn't officially legal (size of void being undefined). Better to do the math on integers and cast to the correct type.

No, then you would be doing arithmetic on a void pointer, which isn't officially legal (size of void being undefined). Better to do the math on integers and cast to the correct type.

It's a GNU C extension which I thought was fine to be used, as we already use other GNU C extensions I think.

No, then you would be doing arithmetic on a void pointer, which isn't officially legal (size of void being undefined). Better to do the math on integers and cast to the correct type.

It's a GNU C extension which I thought was fine to be used, as we already use other GNU C extensions I think.

In practice it should be fine as gcc compatible compilers just assume char-sized arithmetic on void pointers, but you'll definitely get warnings about it, which you then must suppress. :)

So it's probably better safe than sorry, either cast to a byte pointer type like char* or uint8_t*, then do the arithmetic, or go via intptr_t's.