Re: [PATCH 2/7] x86/mkelf32: account for offset when detecting note segment placement

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Mar 19, 2025 at 11:07:33AM +0100, Jan Beulich wrote:
> On 18.03.2025 18:35, Roger Pau Monne wrote:
> > mkelf32 attempt to check that the program header defined NOTE segment falls
> > inside of the LOAD segment, as the build-id should be loaded for Xen at
> > runtime to check.
> > 
> > However the current code doesn't take into account the LOAD program header
> > segment offset when calculating overlap with the NOTE segment.  This
> > results in incorrect detection, and the following build error:
> > 
> > arch/x86/boot/mkelf32 --notes xen-syms ./.xen.elf32 0x200000 \
> >                `nm xen-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$/0x\1/p'`
> > Expected .note section within .text section!
> > Offset 4244776 not within 2910364!
> 
> Not your fault, but: Such printing of decimal numbers is of course very
> unhelpful when ...
> 
> > When xen-syms has the following program headers:
> > 
> > Program Header:
> >     LOAD off    0x0000000000200000 vaddr 0xffff82d040200000 paddr 0x0000000000200000 align 2**21
> >          filesz 0x00000000002c689c memsz 0x00000000003f7e20 flags rwx
> >     NOTE off    0x000000000040c528 vaddr 0xffff82d04040c528 paddr 0x000000000040c528 align 2**2
> >          filesz 0x0000000000000024 memsz 0x0000000000000024 flags r--
> 
> ... any half-way sane tool prints such values in hex.
> 
> > --- a/xen/arch/x86/boot/mkelf32.c
> > +++ b/xen/arch/x86/boot/mkelf32.c
> > @@ -358,7 +358,8 @@ int main(int argc, char **argv)
> >          note_sz = in64_phdr.p_memsz;
> >          note_base = in64_phdr.p_vaddr - note_base;
> >  
> > -        if ( in64_phdr.p_offset > dat_siz || offset > in64_phdr.p_offset )
> > +        if ( in64_phdr.p_offset > (offset + dat_siz) ||
> > +             offset > in64_phdr.p_offset )
> 
> This is an improvement, so fine to go in with Andrew's ack, but it still
> doesn't match what the error message suggests is wanted: This checks only
> whether .note starts after .text or ends before .text. A partial overlap,
> which isn't okay either aiui, would go through fine.
> 
> Oh, and - even in your change there's an off-by-1: You mean >= in the lhs
> of the ||.

Hm, I see, it would be better as:

diff --git a/xen/arch/x86/boot/mkelf32.c b/xen/arch/x86/boot/mkelf32.c
index 5f9e7e440e84..f0f406687cea 100644
--- a/xen/arch/x86/boot/mkelf32.c
+++ b/xen/arch/x86/boot/mkelf32.c
@@ -358,11 +358,13 @@ int main(int argc, char **argv)
         note_sz = in64_phdr.p_memsz;
         note_base = in64_phdr.p_vaddr - note_base;
 
-        if ( in64_phdr.p_offset > dat_siz || offset > in64_phdr.p_offset )
+        if ( in64_phdr.p_offset < offset ||
+             in64_phdr.p_offset + in64_phdr.p_filesz > offset + dat_siz )
         {
             fprintf(stderr, "Expected .note section within .text section!\n" \
-                    "Offset %"PRId64" not within %d!\n",
-                    in64_phdr.p_offset, dat_siz);
+                    ".note: [%"PRIx64", %"PRIx64") .text: [%x, %x)\n",
+                    in64_phdr.p_offset, in64_phdr.p_offset + in64_phdr.p_filesz,
+                    offset, offset + dat_siz);
             return 1;
         }
         /* Gets us the absolute offset within the .text section. */