diff mbox series

[11/20] elf: Fix alloca size in _dl_debug_vdprintf

Message ID 913439ddce2d33024fa0d0da5d9f4c6234a30cde.1666877952.git.szabolcs.nagy@arm.com (mailing list archive)
State Committed
Commit eef17d4d9fcd38c5cbb9bc9515ba72d1773b67a2
Headers
Series patches from the morello port |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Szabolcs Nagy Oct. 27, 2022, 3:33 p.m. UTC 
  The alloca size did not consider the optional width parameter for
padding which could cause buffer underflow. The width is currently used
e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
can be larger than the alloca buffer size on targets where
sizeof(void *) >= 2 * sizeof(unsigned long).

Even if large width is not used on existing targets it is better to fix
the formatting code to avoid surprises.
---
 elf/dl-printf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Florian Weimer Oct. 28, 2022, 5:31 a.m. UTC | #1 
* Szabolcs Nagy via Libc-alpha:

> The alloca size did not consider the optional width parameter for
> padding which could cause buffer underflow. The width is currently used
> e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
> can be larger than the alloca buffer size on targets where
> sizeof(void *) >= 2 * sizeof(unsigned long).
>
> Even if large width is not used on existing targets it is better to fix
> the formatting code to avoid surprises.
> ---
>  elf/dl-printf.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/elf/dl-printf.c b/elf/dl-printf.c
> index 429d2e80c2..00c114002c 100644
> --- a/elf/dl-printf.c
> +++ b/elf/dl-printf.c
> @@ -163,8 +163,11 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
>  		/* We use alloca() to allocate the buffer with the most
>  		   pessimistic guess for the size.  Using alloca() allows
>  		   having more than one integer formatting in a call.  */
> -		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
> -		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
> +		int size = 1 + 3 * sizeof (unsigned long int);
> +		if (width + 1 > size)
> +		  size = width + 1;
> +		char *buf = (char *) alloca (size);
> +		char *endp = &buf[size];
>  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
>  
>  		/* Pad to the width the user specified.  */

This looks okay.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian
Adhemerval Zanella Oct. 28, 2022, 1:56 p.m. UTC | #2 
On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
> The alloca size did not consider the optional width parameter for
> padding which could cause buffer underflow. The width is currently used
> e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
> can be larger than the alloca buffer size on targets where
> sizeof(void *) >= 2 * sizeof(unsigned long).
> 
> Even if large width is not used on existing targets it is better to fix
> the formatting code to avoid surprises.
> ---
>  elf/dl-printf.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/dl-printf.c b/elf/dl-printf.c
> index 429d2e80c2..00c114002c 100644
> --- a/elf/dl-printf.c
> +++ b/elf/dl-printf.c
> @@ -163,8 +163,11 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
>  		/* We use alloca() to allocate the buffer with the most
>  		   pessimistic guess for the size.  Using alloca() allows
>  		   having more than one integer formatting in a call.  */
> -		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
> -		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
> +		int size = 1 + 3 * sizeof (unsigned long int);
> +		if (width + 1 > size)
> +		  size = width + 1;
> +		char *buf = (char *) alloca (size);
> +		char *endp = &buf[size];
>  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
>  
>  		/* Pad to the width the user specified.  */


Would be better to just limit a maximum width and use a fixed-size buffer instead
(and assert if size is larger)?
Szabolcs Nagy Oct. 28, 2022, 2:43 p.m. UTC | #3 
The 10/28/2022 10:56, Adhemerval Zanella Netto wrote:
> On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
> > The alloca size did not consider the optional width parameter for
> > padding which could cause buffer underflow. The width is currently used
> > e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
> > can be larger than the alloca buffer size on targets where
> > sizeof(void *) >= 2 * sizeof(unsigned long).
> > 
> > Even if large width is not used on existing targets it is better to fix
> > the formatting code to avoid surprises.
> > ---
> >  elf/dl-printf.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/elf/dl-printf.c b/elf/dl-printf.c
> > index 429d2e80c2..00c114002c 100644
> > --- a/elf/dl-printf.c
> > +++ b/elf/dl-printf.c
> > @@ -163,8 +163,11 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
> >  		/* We use alloca() to allocate the buffer with the most
> >  		   pessimistic guess for the size.  Using alloca() allows
> >  		   having more than one integer formatting in a call.  */
> > -		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
> > -		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
> > +		int size = 1 + 3 * sizeof (unsigned long int);
> > +		if (width + 1 > size)
> > +		  size = width + 1;
> > +		char *buf = (char *) alloca (size);
> > +		char *endp = &buf[size];
> >  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
> >  
> >  		/* Pad to the width the user specified.  */
> 
> 
> Would be better to just limit a maximum width and use a fixed-size buffer instead
> (and assert if size is larger)? 

i already committed this. i think it's safe:
it's internal api and using huge paddings is unlikely.
Adhemerval Zanella Oct. 28, 2022, 2:48 p.m. UTC | #4 
On 28/10/22 11:43, Szabolcs Nagy wrote:
> The 10/28/2022 10:56, Adhemerval Zanella Netto wrote:
>> On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
>>> The alloca size did not consider the optional width parameter for
>>> padding which could cause buffer underflow. The width is currently used
>>> e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
>>> can be larger than the alloca buffer size on targets where
>>> sizeof(void *) >= 2 * sizeof(unsigned long).
>>>
>>> Even if large width is not used on existing targets it is better to fix
>>> the formatting code to avoid surprises.
>>> ---
>>>  elf/dl-printf.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/elf/dl-printf.c b/elf/dl-printf.c
>>> index 429d2e80c2..00c114002c 100644
>>> --- a/elf/dl-printf.c
>>> +++ b/elf/dl-printf.c
>>> @@ -163,8 +163,11 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
>>>  		/* We use alloca() to allocate the buffer with the most
>>>  		   pessimistic guess for the size.  Using alloca() allows
>>>  		   having more than one integer formatting in a call.  */
>>> -		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
>>> -		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
>>> +		int size = 1 + 3 * sizeof (unsigned long int);
>>> +		if (width + 1 > size)
>>> +		  size = width + 1;
>>> +		char *buf = (char *) alloca (size);
>>> +		char *endp = &buf[size];
>>>  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
>>>  
>>>  		/* Pad to the width the user specified.  */
>>
>>
>> Would be better to just limit a maximum width and use a fixed-size buffer instead
>> (and assert if size is larger)? 
> 
> i already committed this. i think it's safe:
> it's internal api and using huge paddings is unlikely.

The idea is to remove all internal alloca usage where possible,
and I think this is a good spot to continue the work.
diff mbox series

Patch

diff --git a/elf/dl-printf.c b/elf/dl-printf.c
index 429d2e80c2..00c114002c 100644
--- a/elf/dl-printf.c
+++ b/elf/dl-printf.c
@@ -163,8 +163,11 @@  _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
 		/* We use alloca() to allocate the buffer with the most
 		   pessimistic guess for the size.  Using alloca() allows
 		   having more than one integer formatting in a call.  */
-		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
-		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
+		int size = 1 + 3 * sizeof (unsigned long int);
+		if (width + 1 > size)
+		  size = width + 1;
+		char *buf = (char *) alloca (size);
+		char *endp = &buf[size];
 		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
 
 		/* Pad to the width the user specified.  */