diff mbox series

[v2] elf: Check corrupt VTENTRY relocation overflow

Message ID CAMe9rOpdXsCvOwbDAPGuR9oEG2-h2qVCkU4yHm5G3dXs7NqU_Q@mail.gmail.com
State New
Headers
Series [v2] elf: Check corrupt VTENTRY relocation overflow |

Commit Message

H.J. Lu Sept. 23, 2025, 9:25 p.m. UTC 
  On Tue, Sep 23, 2025 at 10:42 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.09.2025 13:57, Alan Modra wrote:
> > On Tue, Sep 23, 2025 at 10:16:50AM +0800, H.J. Lu wrote:
> >> --- a/bfd/elflink.c
> >> +++ b/bfd/elflink.c
> >> @@ -14895,6 +14895,15 @@ bfd_elf_gc_record_vtentry (bfd *abfd, asection *sec,
> >>      }
> >>        size = (size + file_align - 1) & -file_align;
> >>
> >> +      if (addend > size)
> >
> > For this to happen you must have had an overflow in prior expressions
> > calculating size from addend, I think.  Perhaps it might be better to
> > limit vtentry reloc addends to something a lot smaller than -8ul
> > (which I think is effectively what your patch does), to prevent insane
> > vtable memory allocation.  What is a reasonable limit to c++ vtable
> > size?
>
> Can we really build in a heuristic like that? Arbitrarily complex class
> hierarchies can have arbitrarily large vtables, I suppose.
>
> Jan

Here is the v2 patch to check the size calculation overflow instead.
OK for master?

Thanks.

Comments

Jan Beulich Sept. 25, 2025, 6 a.m. UTC | #1 
On 23.09.2025 23:25, H.J. Lu wrote:
> On Tue, Sep 23, 2025 at 10:42 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 23.09.2025 13:57, Alan Modra wrote:
>>> On Tue, Sep 23, 2025 at 10:16:50AM +0800, H.J. Lu wrote:
>>>> --- a/bfd/elflink.c
>>>> +++ b/bfd/elflink.c
>>>> @@ -14895,6 +14895,15 @@ bfd_elf_gc_record_vtentry (bfd *abfd, asection *sec,
>>>>      }
>>>>        size = (size + file_align - 1) & -file_align;
>>>>
>>>> +      if (addend > size)
>>>
>>> For this to happen you must have had an overflow in prior expressions
>>> calculating size from addend, I think.  Perhaps it might be better to
>>> limit vtentry reloc addends to something a lot smaller than -8ul
>>> (which I think is effectively what your patch does), to prevent insane
>>> vtable memory allocation.  What is a reasonable limit to c++ vtable
>>> size?
>>
>> Can we really build in a heuristic like that? Arbitrarily complex class
>> hierarchies can have arbitrarily large vtables, I suppose.
> 
> Here is the v2 patch to check the size calculation overflow instead.
> OK for master?

Isn't this superseded by Alan's patch?

Jan
H.J. Lu Sept. 25, 2025, 6:29 a.m. UTC | #2 
On Thu, Sep 25, 2025 at 2:00 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.09.2025 23:25, H.J. Lu wrote:
> > On Tue, Sep 23, 2025 at 10:42 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 23.09.2025 13:57, Alan Modra wrote:
> >>> On Tue, Sep 23, 2025 at 10:16:50AM +0800, H.J. Lu wrote:
> >>>> --- a/bfd/elflink.c
> >>>> +++ b/bfd/elflink.c
> >>>> @@ -14895,6 +14895,15 @@ bfd_elf_gc_record_vtentry (bfd *abfd, asection *sec,
> >>>>      }
> >>>>        size = (size + file_align - 1) & -file_align;
> >>>>
> >>>> +      if (addend > size)
> >>>
> >>> For this to happen you must have had an overflow in prior expressions
> >>> calculating size from addend, I think.  Perhaps it might be better to
> >>> limit vtentry reloc addends to something a lot smaller than -8ul
> >>> (which I think is effectively what your patch does), to prevent insane
> >>> vtable memory allocation.  What is a reasonable limit to c++ vtable
> >>> size?
> >>
> >> Can we really build in a heuristic like that? Arbitrarily complex class
> >> hierarchies can have arbitrarily large vtables, I suppose.
> >
> > Here is the v2 patch to check the size calculation overflow instead.
> > OK for master?
>
> Isn't this superseded by Alan's patch?
>
> Jan

I think so.
diff mbox series

Patch

From 2a592212e23ac0f06534fcec74f3633272237310 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 23 Sep 2025 05:35:09 +0800
Subject: [PATCH v2] elf: Check corrupt VTENTRY relocation overflow

If VTENTRY relocation addend is too large, the size calculation will
overflow.  Check it to avoid linker crash later.

	PR ld/33452
	* elflink.c (bfd_elf_gc_record_vtentry): Return false if VTENTRY
	relocation addend causes the size calculation overflow.
---
 bfd/elflink.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 54f0d6e957e..17b77201ff9 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -14891,7 +14891,18 @@  bfd_elf_gc_record_vtentry (bfd *abfd, asection *sec,
 	 a zero size.  */
       file_align = 1 << log_file_align;
       if (h->root.type == bfd_link_hash_undefined)
-	size = addend + file_align;
+	{
+	  size = addend + file_align;
+	  /* Check for size overflow.  */
+	  if (addend > size || file_align > size)
+	    {
+	      /* xgettext:c-format */
+	      _bfd_error_handler (_("%pB: section '%pA': corrupt VTENTRY relocation"),
+				  abfd, sec);
+	      bfd_set_error (bfd_error_bad_value);
+	      return false;
+	    }
+	}
       else
 	{
 	  size = h->size;
-- 
2.51.0