Re: [Xen-devel] [PATCH v11 9/9] xen: explicit casts when DECLARE_BOUNDS cannot be used [and 1 more messages]

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 7 Mar 2019, Ian Jackson wrote:
> bug_frames
> ----------
> 
> What appears to be going on is this:
> 
> setup_virtual_regions contains a static const array of pointers to
> struct bug_frame.  These struct bug_frame* values are themselves
> linker symbol values.
> 
> Because the compiler has visibility of all of this, it is allowed to
> assume that these values do not change.  It will therefore wrongly
> assume that they are incomparable.
> 
> (All of this ought to have been explained in comments in the code.
> Given that it wasn't, a comment ought to have been in your patch.
> I shouldn't have had to go and dig into the code.)
> 
> And I'm afraid I do not like your patch.  The declaration in eg
> xen/include/asm-arm/bug.h
> 
>   extern const struct bug_frame __start_bug_frames[],
>                                 __stop_bug_frames_0[],
>                                 __stop_bug_frames_1[],
>                                 __stop_bug_frames_2[];
> 
> is a clear violation of the rule in the DECLARE_BOUNDS comment:
> 
> +  * These macros, or an alternative technique, MUST be used any time
> +  * linker symbols are imported into C via the `extern []' idiom.
> 
> I think an `alternative technique' should be applied *at the time of
> the declaration* and it should suffice to *mostly avoid the risk of
> dangerous uses*.
> 
> Jan has one suggestion, which I don't fully understand (see below).
> Alternatively I suggest the following ad-hoc approach:
> 
>   * Rename __start_bug_frames etc. to
>       __UBDANGER_*bug_frame*
>     (and provide an accompanying comment saying what the UB danger is
>     and how UB must be avoided).
> 
>   * Change the definition of bug_frames to
>       static const uintptr_t __initconstrel bug_frames[] ...
> 
>   * Provide an arch-independent helper macro to both declare
>     __UBDANGER_*bug_frame*, converting the pointers to
>     uintptr_t, and initialise the array.

I don't follow why this macro is needed and what more concretely you
have in mind for the array initialization.


>   * In setup_virtual_regions, cast the uintptr_t to a pointer.  This
>     needs to be accompanied by a substantial comment explaining why
>     this is safe.  Elements of the proof are
>       - stating that the thing came from a pointer which came
>         from a linker symbol, so it is a valid pointer value
>       - some kind of argument that no code elsewhere will
>         compare pointers from different core_init.frame
>         and core_init.ex entries.
>     I observe that with the current code I think making such an
>     argument may involve doing something about core_init.ex and
>     core_init.ex_end.  Not sure.

I am sure it will take a couple of tries to get the comment right, but I
can give a try at a first draft.


> An alternative would be to construct this table in a different
> translation unit, in which case the compiler compiling
> setup_virtual_regions cannot `prove' the falsehood .  (Note that we
> must be disabling whole-program optimisation.  I hope we are!)
> 
> 
> Jan writes:
> 
> > I disagree with the comment,
> 
> I also disagree with the wording of the comment.  It is seriously
> misleading.  These symbols do in fact refer to the same object!
> The problem is that the compiler thinks otherwise.  You need wording
> like that in DECLARE_BOUNDS.  (Or a reference to it.)
> 
> > and if you think it is correct, then no
> > matter what you do the behavior is undefined. Instead I view the
> > entirety of the .bug_frames.* sections as a single array, with
> > labels placed not only at start and end, but also in the middle. I
> > think the code here would better also be taken care of by the
> > DECLARE_BOUNDS() machinery, dividing the single array into
> > multiple smaller ones.
> 
> Jan, I'm not sure exactly what you are suggesting.  Currently the
> array has one pointer per element.  Are you suggesting it should have
> two pointers (start and end), with different notional types ?

I am not fully understanding the suggestion either.

Even if we split it into multiple start-end pairs, still we won't be
able to compare start/end of a pair with start/end of different pair
without casts.


> If that is OK from a perf point of view then it is an easy answer
> (although a bit tiresome since more linker symbols will have to be
> generated).
> 
> 
> __start_xen
> -----------
> 
> > @@ -976,7 +976,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >           * respective reserve_e820_ram() invocation below.
> >           */
> >          mod[mbi->mods_count].mod_start = virt_to_mfn(_stext);
> > -        mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext;
> > +        mod[mbi->mods_count].mod_end = (uintptr_t)__2M_rwdata_end -
> > +                                       (uintptr_t) _stext;
> 
> __2M_rwdata_end and _stext are both declared in header files using the
> deprecated pattern.
> 
>   xen/include/xen/kernel.h:extern char _stext[], _etext[];
>   xen/include/asm-x86/setup.h:extern char __2M_rwdata_start[], 
> __2M_rwdata_end[];
> 
> According to the comment for DEFINE_BOUNDS, it ought to be used here,
> or some other mechanism.  But AFAICT hyou have not changed the
> declarations ?
> 
> If you changed the declarations then (i) mistakes would be avoided
> (ii) you would still have to use explicit casts to compare these
> pointers to different sections, but you could write a clear
> explanation of (a) why this is needed (b) why it is safe.
> 
> You have done neither.
> 
> 
> I hope this makes sense.
 
This makes sense. Also, I understand _stext and __2M_rwdata_end better
than the bug_frames and I should be able to write an half-decent
explanation here. FYI _stext is already converted to DEFINE_BOUNDS in
this series, but you are right that __2M_rwdata_end is not. I'll fix
that.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel