| Message ID | 913439ddce2d33024fa0d0da5d9f4c6234a30cde.1666877952.git.szabolcs.nagy@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | patches from the morello port | expand |
* 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
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)?
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.
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 --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. */