Patchwork [U-Boot,RFC,3/3] arm920t: do not relocate NULL pointer

login
register
mail settings
Submitter Andreas Bießmann
Date Nov. 30, 2010, 7:06 a.m.
Message ID <1291100800-61850-4-git-send-email-andreas.devel@googlemail.com>
Download mbox | patch
Permalink /patch/73578/
State RFC
Headers show

Comments

Andreas Bießmann - Nov. 30, 2010, 7:06 a.m.
Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com>
---
 arch/arm/cpu/arm920t/start.S |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Albert ARIBAUD - Nov. 30, 2010, 8:32 a.m.
Le 30/11/2010 08:06, Andreas Bießmann a écrit :
> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com>

> +	cmp	r1, #0			/* symbol == NULL ? */
> +	beq	fixnext

Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since 
they are a constant. If a NULL ends up in relocation tables, that is 
because of a corruption *or* because it was to be relocated, and should 
thus never be ignored.

Amicalement,
Joakim Tjernlund - Nov. 30, 2010, 8:47 a.m.
>
> Le 30/11/2010 08:06, Andreas Bießmann a écrit :
> > Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com>
>
> > +   cmp   r1, #0         /* symbol == NULL ? */
> > +   beq   fixnext
>
> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
> they are a constant. If a NULL ends up in relocation tables, that is
> because of a corruption *or* because it was to be relocated, and should
> thus never be ignored.

Depends, if the same routine is used for relocating fixups you need
this test. Undefined weaks will generate a NULL fixup entry.

    Jocke
Andreas Bießmann - Nov. 30, 2010, 8:50 a.m.
Dear All,

Am 30.11.2010 09:47, schrieb Joakim Tjernlund:
>>
>> Le 30/11/2010 08:06, Andreas Bießmann a écrit :
>>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com>
>>
>>> +   cmp   r1, #0         /* symbol == NULL ? */
>>> +   beq   fixnext
>>
>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
>> they are a constant. If a NULL ends up in relocation tables, that is
>> because of a corruption *or* because it was to be relocated, and should
>> thus never be ignored.
> 
> Depends, if the same routine is used for relocating fixups you need
> this test.

> Undefined weaks will generate a NULL fixup entry.

As mentioned by Jens and one other some time ago. Albert, please build
e.g. at91rm9200ek boards and search for board_reset(), defined in
arch/arm/cpu/arm920t/at91/reset.c

regards

Andreas Bießmann
Albert ARIBAUD - Nov. 30, 2010, 9:02 a.m.
Le 30/11/2010 09:47, Joakim Tjernlund a écrit :
>>
>> Le 30/11/2010 08:06, Andreas Bießmann a écrit :
>>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com>
>>
>>> +   cmp   r1, #0         /* symbol == NULL ? */
>>> +   beq   fixnext
>>
>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
>> they are a constant. If a NULL ends up in relocation tables, that is
>> because of a corruption *or* because it was to be relocated, and should
>> thus never be ignored.
>
> Depends, if the same routine is used for relocating fixups you need
> this test. Undefined weaks will generate a NULL fixup entry.

Understood.

Weren't there an effort to not use weak symbols any more?

If not, then a comment *must* be added to indicate that weak symbols can 
cause zero-filled reloc entries (I would suggest saying 'zero-filled' 
rather than 'NULL', because obviously readers will think of stdio's NULL).

We could possibly even send out a diagnostic message, but that'll be 
when the relocation code is turned to C language; I don't want to see 
asm code that calls printf.

>      Jocke

Amicalement,
Joakim Tjernlund - Nov. 30, 2010, 9:41 a.m.
Albert ARIBAUD <albert@aribaud.net> wrote on 2010/11/30 10:02:45:
>
> Le 30/11/2010 09:47, Joakim Tjernlund a écrit :
> >>
> >> Le 30/11/2010 08:06, Andreas Bießmann a écrit :
> >>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com>
> >>
> >>> +   cmp   r1, #0         /* symbol == NULL ? */
> >>> +   beq   fixnext
> >>
> >> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
> >> they are a constant. If a NULL ends up in relocation tables, that is
> >> because of a corruption *or* because it was to be relocated, and should
> >> thus never be ignored.
> >
> > Depends, if the same routine is used for relocating fixups you need
> > this test. Undefined weaks will generate a NULL fixup entry.
>
> Understood.

note that I don't know how this routine is used so if just
relocates the GOT you don't need to test for NULL.

>
> Weren't there an effort to not use weak symbols any more?

ehh, not what I am aware of. Just that we should not use(ATM)
undefined weaks.

>
> If not, then a comment *must* be added to indicate that weak symbols can
> cause zero-filled reloc entries (I would suggest saying 'zero-filled'
> rather than 'NULL', because obviously readers will think of stdio's NULL).

zero-filled/NULL fixup entries. The GOT never holds NULL

>
> We could possibly even send out a diagnostic message, but that'll be
> when the relocation code is turned to C language; I don't want to see
> asm code that calls printf.

No need really.
Andreas Bießmann - Nov. 30, 2010, 11:48 a.m.
Dear Joakim Tjernlund,
Dear Albert ARIBAUD,

Am 30.11.2010 10:41, schrieb Joakim Tjernlund:
> Albert ARIBAUD <albert@aribaud.net> wrote on 2010/11/30 10:02:45:
>>
>> Le 30/11/2010 09:47, Joakim Tjernlund a écrit :
>>>>
>>>> Le 30/11/2010 08:06, Andreas Bießmann a écrit :
>>>>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com>
>>>>
>>>>> +   cmp   r1, #0         /* symbol == NULL ? */
>>>>> +   beq   fixnext
>>>>
>>>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
>>>> they are a constant. If a NULL ends up in relocation tables, that is
>>>> because of a corruption *or* because it was to be relocated, and should
>>>> thus never be ignored.
>>>
>>> Depends, if the same routine is used for relocating fixups you need
>>> this test. Undefined weaks will generate a NULL fixup entry.
>>
>> Understood.
> 
> note that I don't know how this routine is used so if just
> relocates the GOT you don't need to test for NULL.
> 
>>
>> Weren't there an effort to not use weak symbols any more?
> 
> ehh, not what I am aware of. Just that we should not use(ATM)
> undefined weaks.

Ok, I did forget to investigate that as mentioned in
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/88024/focus=88324

Here are the results:

Currently arm920/at91 devices have one undefined weak symbol in .dynsym:

---8<---
Symbol table '.dynsym' contains 11 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND <corrupt>
     1: 10000000     0 SECTION LOCAL  DEFAULT    1 <corrupt>
     2: 10028a28     0 SECTION LOCAL  DEFAULT    6 <corrupt>
     3: 10029ff8     0 NOTYPE  GLOBAL DEFAULT    9 <corrupt>
     4: 100299f4     0 NOTYPE  GLOBAL DEFAULT  ABS <corrupt>
     5: 00000000     0 NOTYPE  WEAK   DEFAULT  UND <corrupt>
     6: 10029ff8     0 NOTYPE  GLOBAL DEFAULT  ABS <corrupt>
     7: 10029ff8     0 NOTYPE  GLOBAL DEFAULT   11 <corrupt>
     8: 1002edf0     0 NOTYPE  GLOBAL DEFAULT   10 <corrupt>
     9: 100701c0     0 NOTYPE  GLOBAL DEFAULT   11 <corrupt>
    10: 1002edf0     0 NOTYPE  GLOBAL DEFAULT    9 <corrupt>
--->8---

---8<---
Hex dump of section '.dynsym':
  0x1002edf0 00000000 00000000 00000000 00000000 ................
  0x1002ee00 00000000 00000010 00000000 03000100 ................
  0x1002ee10 00000000 288a0210 00000000 03000600 ....(...........
  0x1002ee20 25000000 f89f0210 00000000 10000900 %...............
  0x1002ee30 01000000 f4990210 00000000 1000f1ff ................
  0x1002ee40 5e000000 00000000 00000000 20000000 ^........... ...
  0x1002ee50 14000000 f89f0210 00000000 1000f1ff ................
  0x1002ee60 52000000 f89f0210 00000000 10000b00 R...............
  0x1002ee70 43000000 f0ed0210 00000000 10000a00 C...............
  0x1002ee80 20000000 c0010710 00000000 10000b00  ...............
  0x1002ee90 35000000 f0ed0210 00000000 10000900 5...............
--->8---

So there are two options here.

First is to apply '[PATCH v2 1/2] arm920t/at91/reset.c: fix weak
reset_board()' second (and safer) is to apply (maybe modified) '[PATCH
RFC 3/3] arm920t: do not relocate NULL pointer'.

which one is preferred? Or should we do both?

regards

Andreas Bießmann
Joakim Tjernlund - Nov. 30, 2010, 11:56 a.m.
"Andreas Bießmann" <andreas.devel@googlemail.com> wrote on 2010/11/30 12:48:41:
>
> Dear Joakim Tjernlund,
> Dear Albert ARIBAUD,
>
> Am 30.11.2010 10:41, schrieb Joakim Tjernlund:
> > Albert ARIBAUD <albert@aribaud.net> wrote on 2010/11/30 10:02:45:
> >>
> >> Le 30/11/2010 09:47, Joakim Tjernlund a écrit :
> >>>>
> >>>> Le 30/11/2010 08:06, Andreas Bießmann a écrit :
> >>>>> Signed-off-by: Andreas Bießmann<andreas.devel@googlemail.com>
> >>>>
> >>>>> +   cmp   r1, #0         /* symbol == NULL ? */
> >>>>> +   beq   fixnext
> >>>>
> >>>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
> >>>> they are a constant. If a NULL ends up in relocation tables, that is
> >>>> because of a corruption *or* because it was to be relocated, and should
> >>>> thus never be ignored.
> >>>
> >>> Depends, if the same routine is used for relocating fixups you need
> >>> this test. Undefined weaks will generate a NULL fixup entry.
> >>
> >> Understood.
> >
> > note that I don't know how this routine is used so if just
> > relocates the GOT you don't need to test for NULL.
> >
> >>
> >> Weren't there an effort to not use weak symbols any more?
> >
> > ehh, not what I am aware of. Just that we should not use(ATM)
> > undefined weaks.
>
> Ok, I did forget to investigate that as mentioned in
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/88024/focus=88324
>
> Here are the results:
>
> Currently arm920/at91 devices have one undefined weak symbol in .dynsym:
>
> ---8<---
> Symbol table '.dynsym' contains 11 entries:
>    Num:    Value  Size Type    Bind   Vis      Ndx Name
>      0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND <corrupt>
>      1: 10000000     0 SECTION LOCAL  DEFAULT    1 <corrupt>
>      2: 10028a28     0 SECTION LOCAL  DEFAULT    6 <corrupt>
>      3: 10029ff8     0 NOTYPE  GLOBAL DEFAULT    9 <corrupt>
>      4: 100299f4     0 NOTYPE  GLOBAL DEFAULT  ABS <corrupt>
>      5: 00000000     0 NOTYPE  WEAK   DEFAULT  UND <corrupt>
>      6: 10029ff8     0 NOTYPE  GLOBAL DEFAULT  ABS <corrupt>
>      7: 10029ff8     0 NOTYPE  GLOBAL DEFAULT   11 <corrupt>
>      8: 1002edf0     0 NOTYPE  GLOBAL DEFAULT   10 <corrupt>
>      9: 100701c0     0 NOTYPE  GLOBAL DEFAULT   11 <corrupt>
>     10: 1002edf0     0 NOTYPE  GLOBAL DEFAULT    9 <corrupt>
> --->8---
>
> ---8<---
> Hex dump of section '.dynsym':
>   0x1002edf0 00000000 00000000 00000000 00000000 ................
>   0x1002ee00 00000000 00000010 00000000 03000100 ................
>   0x1002ee10 00000000 288a0210 00000000 03000600 ....(...........
>   0x1002ee20 25000000 f89f0210 00000000 10000900 %...............
>   0x1002ee30 01000000 f4990210 00000000 1000f1ff ................
>   0x1002ee40 5e000000 00000000 00000000 20000000 ^........... ...
>   0x1002ee50 14000000 f89f0210 00000000 1000f1ff ................
>   0x1002ee60 52000000 f89f0210 00000000 10000b00 R...............
>   0x1002ee70 43000000 f0ed0210 00000000 10000a00 C...............
>   0x1002ee80 20000000 c0010710 00000000 10000b00  ...............
>   0x1002ee90 35000000 f0ed0210 00000000 10000900 5...............
> --->8---
>
> So there are two options here.
>
> First is to apply '[PATCH v2 1/2] arm920t/at91/reset.c: fix weak
> reset_board()' second (and safer) is to apply (maybe modified) '[PATCH
> RFC 3/3] arm920t: do not relocate NULL pointer'.
>
> which one is preferred? Or should we do both?

Both, you don't know what the future will be and perhaps someone
wants to use undef weaks in board code.

   Jocke

Patch

diff --git a/arch/arm/cpu/arm920t/start.S b/arch/arm/cpu/arm920t/start.S
index 629be3f..e3f9cdb 100644
--- a/arch/arm/cpu/arm920t/start.S
+++ b/arch/arm/cpu/arm920t/start.S
@@ -248,11 +248,15 @@  fixabs:
 	mov	r1, r1, LSR #4		/* r1 <- symbol index in .dynsym */
 	add	r1, r10, r1		/* r1 <- address of symbol in table */
 	ldr	r1, [r1, #4]		/* r1 <- symbol value */
+	cmp	r1, #0			/* symbol == NULL ? */
+	beq	fixnext
 	add	r1, r9			/* r1 <- relocated sym addr */
 	b	fixnext
 fixrel:
 	/* relative fix: increase location by offset */
 	ldr	r1, [r0]		/* r1 <- address of symbol */
+	cmp	r1, #0			/* symbol == NULL ? */
+	beq	fixnext
 	add	r1, r1, r9		/* r1 <- relocated address of symbol */
 fixnext:
 	str	r1, [r0]		/* store back content of r1 */