Re: GRUB Coverity x86_64/EFI and ARM64/EFI runs - 0 outstanding defects
Hi Daniel, Great to know the target was reached. The last set of issues seemed to be a real challenge. You set a high bar as a maintainer, encouraging everyone towards a resolution and making GRUB all the better for it. Well done all! Thanks, Darren. On Tuesday, 2023-11-28 at 17:59:31 +01, Daniel Kiper wrote: > Hi, > > It is our pleasure to inform you that we were able to achieve 0 outstanding > defects for the GRUB Coverity x86_64/EFI and ARM64/EFI runs. We did analysis > of 629 issues and fixed 535 of them. The rest has been dismissed mostly as > false positives. This work allowed us to improve overall GRUB code quality, > reliability and security. We will continue using the Coverity to verify > correctness of newly introduced code and existing one. > > The Coverity analysis and fixing of reported issues was initiated by Andrei > Borzenkov in 2014. Later Vladimir Serbinenko joined and together continued > this > work until 2017. We restarted using the Coverity analyzer in 2020, when we > were > fixing BootHole security vulnerability and other issues. It took us 3 years to > do analysis of all Coverity issues not solved earlier. It was very tedious > process requiring hundreds of hours of code analysis done by many people. We > think it is important to name all, in alphabetical order, who tirelessly > worked > to null out all defects reported by the Coverity: > - Alec Brown (Oracle), > - Alexey Makhalov (VMware), > - Andrei Borzenkov, > - Chris Coulson (Canonical), > - Daniel Axtens, > - Darren Kenny (Oracle), > - Glenn Washburn, > - Jagannathan Raman (Oracle), > - Jan Setje-Eilers (Oracle), > - Konrad Rzeszutek Wilk (Oracle), > - Marco A Benatto (Red Hat), > - Patrick Steinhardt, > - Paulo Flabiano Smorigo (Canonical), > - Ross Philipson (Oracle), > - Vladimir Serbinenko, > - WANG Xuerui. > > Thank you for doing this work guys! This success would not be possible > without you! > > Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 1/1] fs/udf: Fix out of bounds access
Hi Li,,
LGTM!
Reviewed-by: Darren Kenny
Thanks,
Darren.
On Wednesday, 2023-06-07 at 01:31:06 UTC, Lidong Chen wrote:
> Implemented a boundary check before advancing the allocation
> descriptors pointer.
>
> Signed-off-by: Lidong Chen
> Reviewed-by: Darren Kenny
> Reviewed-by: Daniel Kiper
> ---
> grub-core/fs/udf.c | 38 ++
> 1 file changed, 38 insertions(+)
>
> diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
> index 7679ea309..58884d2ba 100644
> --- a/grub-core/fs/udf.c
> +++ b/grub-core/fs/udf.c
> @@ -114,6 +114,10 @@ GRUB_MOD_LICENSE ("GPLv3+");
> #define GRUB_UDF_PARTMAP_TYPE_1 1
> #define GRUB_UDF_PARTMAP_TYPE_2 2
>
> +#define GRUB_UDF_INVALID_STRUCT_PTR(_ptr, _struct) \
> + ((char *) (_ptr) >= end_ptr || \
> + ((grub_ssize_t)(end_ptr - (char*)(_ptr)) < (grub_ssize_t)sizeof(_struct)))
> +
> struct grub_udf_lb_addr
> {
>grub_uint32_t block_num;
> @@ -458,6 +462,7 @@ grub_udf_read_block (grub_fshelp_node_t node,
> grub_disk_addr_t fileblock)
>char *ptr;
>grub_ssize_t len;
>grub_disk_addr_t filebytes;
> + char *end_ptr;
>
>switch (U16 (node->block.fe.tag.tag_ident))
> {
> @@ -476,9 +481,17 @@ grub_udf_read_block (grub_fshelp_node_t node,
> grub_disk_addr_t fileblock)
>return 0;
> }
>
> + end_ptr = (char *) node + get_fshelp_size (node->data);
> +
>if ((U16 (node->block.fe.icbtag.flags) & GRUB_UDF_ICBTAG_FLAG_AD_MASK)
>== GRUB_UDF_ICBTAG_FLAG_AD_SHORT)
> {
> + if (GRUB_UDF_INVALID_STRUCT_PTR(ptr, struct grub_udf_short_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> +
>struct grub_udf_short_ad *ad = (struct grub_udf_short_ad *) ptr;
>
>filebytes = fileblock * U32 (node->data->lvd.bsize);
> @@ -542,10 +555,22 @@ grub_udf_read_block (grub_fshelp_node_t node,
> grub_disk_addr_t fileblock)
> filebytes -= adlen;
> ad++;
> len -= sizeof (struct grub_udf_short_ad);
> +
> + if (GRUB_UDF_INVALID_STRUCT_PTR(ad, struct grub_udf_short_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> }
> }
>else
> {
> + if (GRUB_UDF_INVALID_STRUCT_PTR(ptr, struct grub_udf_long_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> +
>struct grub_udf_long_ad *ad = (struct grub_udf_long_ad *) ptr;
>
>filebytes = fileblock * U32 (node->data->lvd.bsize);
> @@ -611,6 +636,12 @@ grub_udf_read_block (grub_fshelp_node_t node,
> grub_disk_addr_t fileblock)
> filebytes -= adlen;
> ad++;
> len -= sizeof (struct grub_udf_long_ad);
> +
> + if (GRUB_UDF_INVALID_STRUCT_PTR(ad, struct grub_udf_long_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> }
> }
>
> @@ -630,6 +661,7 @@ grub_udf_read_file (grub_fshelp_node_t node,
> case GRUB_UDF_ICBTAG_FLAG_AD_IN_ICB:
>{
> char *ptr;
> + char *end_ptr = (char *) node + get_fshelp_size (node->data);
>
> ptr = ((U16 (node->block.fe.tag.tag_ident) == GRUB_UDF_TAG_IDENT_FE) ?
> ((char *) >block.fe.ext_attr[0]
> @@ -637,6 +669,12 @@ grub_udf_read_file (grub_fshelp_node_t node,
> ((char *) >block.efe.ext_attr[0]
> + U32 (node->block.efe.ext_attr_length)));
>
> + if ((ptr + pos + len) > end_ptr)
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> +
> grub_memcpy (buf, ptr + pos, len);
>
> return len;
> --
> 2.39.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/1] fs/udf: Fix out of bounds access
Hi Li,
In general looks good...
On Thursday, 2023-06-01 at 18:50:19 UTC, Lidong Chen wrote:
> Implemented a boundary check before advancing the allocation
> descriptors pointer.
>
> Signed-off-by: Lidong Chen
> ---
> grub-core/fs/udf.c | 36
> 1 file changed, 36 insertions(+)
>
> diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
> index 12e88ab62..2359222eb 100644
> --- a/grub-core/fs/udf.c
> +++ b/grub-core/fs/udf.c
> @@ -458,6 +458,7 @@ grub_udf_read_block (grub_fshelp_node_t node,
> grub_disk_addr_t fileblock)
>char *ptr;
>grub_ssize_t len;
>grub_disk_addr_t filebytes;
> + char *end_ptr;
>
>switch (U16 (node->block.fe.tag.tag_ident))
> {
> @@ -476,9 +477,17 @@ grub_udf_read_block (grub_fshelp_node_t node,
> grub_disk_addr_t fileblock)
>return 0;
> }
>
> + end_ptr = (char *) node + get_fshelp_size (node->data);
> +
>if ((U16 (node->block.fe.icbtag.flags) & GRUB_UDF_ICBTAG_FLAG_AD_MASK)
>== GRUB_UDF_ICBTAG_FLAG_AD_SHORT)
> {
> + if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_short_ad))
>
Should this probably also be testing ptr < end_ptr?
I wonder if a local macro like this would be useful:
#define GRUB_UDF_INVALID_STRUCT_PTR(_ptr, _struct) \
((char *) (_ptr) >= end_ptr ||
((grub_ssize_t)(end_ptr - (char*)(_ptr)) < (grub_ssize_t)sizeof(_struct))
or the more positive and succinct version, and subsequent negated (!) test:
#define GRUB_UDF_VALID_STRUCT_PTR(_ptr, _struct) \
((char *)(_ptr) <= (end_ptr - sizeof(_struct)))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> +
>struct grub_udf_short_ad *ad = (struct grub_udf_short_ad *) ptr;
>
>filebytes = fileblock * U32 (node->data->lvd.bsize);
> @@ -528,10 +537,23 @@ grub_udf_read_block (grub_fshelp_node_t node,
> grub_disk_addr_t fileblock)
> filebytes -= adlen;
> ad++;
> len -= sizeof (struct grub_udf_short_ad);
> +
> + if ((char *) ad >= end_ptr ||
> + (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct
> grub_udf_short_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> }
> }
>else
> {
> + if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_long_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> +
>struct grub_udf_long_ad *ad = (struct grub_udf_long_ad *) ptr;
>
>filebytes = fileblock * U32 (node->data->lvd.bsize);
> @@ -583,6 +605,13 @@ grub_udf_read_block (grub_fshelp_node_t node,
> grub_disk_addr_t fileblock)
> filebytes -= adlen;
> ad++;
> len -= sizeof (struct grub_udf_long_ad);
> +
> + if ((char *) ad >= end_ptr ||
> + (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct
> grub_udf_long_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> }
> }
>
> @@ -602,6 +631,7 @@ grub_udf_read_file (grub_fshelp_node_t node,
> case GRUB_UDF_ICBTAG_FLAG_AD_IN_ICB:
>{
> char *ptr;
> + char *end_ptr = (char *) node + get_fshelp_size (node->data);
>
> ptr = ((U16 (node->block.fe.tag.tag_ident) == GRUB_UDF_TAG_IDENT_FE) ?
> ((char *) >block.fe.ext_attr[0]
> @@ -609,6 +639,12 @@ grub_udf_read_file (grub_fshelp_node_t node,
> ((char *) >block.efe.ext_attr[0]
> + U32 (node->block.efe.ext_attr_length)));
>
> + if (ptr > end_ptr || (ptr + pos) > end_ptr || (ptr + pos + len) >
> end_ptr)
>
Not sure there is a need for all of these, would the last one not
suffice? Might be worth testing that pos and len are > 0 if only using
that one.
Thanks,
Darren.
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> +
> grub_memcpy (buf, ptr + pos, len);
>
> return len;
> --
> 2.39.1
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] video/readers: Add artificial limit to image dimensions
Hi Alec,
On Thursday, 2022-10-27 at 01:16:44 +01, Alec Brown wrote:
> In grub-core/video/readers/jpeg.c, the height and width of a JPEG image don't
> have an upper limit for how big the JPEG image can be. In coverity, this is
> getting flagged as an untrusted loop bound. This issue can also seen in PNG
> and
> TGA format images as well but coverity isn't flagging it. To prevent this, the
> constant IMAGE_HW_MAX_PX is being added to bitmap.h, which has a value of
> 16384,
> to act as an artifical limit and restrict the height and width of images. This
> value was picked as it is double the current max resolution size, which is 8K.
>
> Fixes: CID 292450
>
> Signed-off-by: Alec Brown
>
Looks good to me, so:
Reviewed-by: Darren Kenny
Thanks,
Darren.
> ---
>
> In v1, the patch set was developed on outdated code and there was
> already a fix for the second patch. So in this version, the second patch
> has been dropped. The only thing that has changed in this patch is line
> numbers.
>
> docs/grub.texi | 3 ++-
> grub-core/video/readers/jpeg.c | 6 +-
> grub-core/video/readers/png.c | 6 +-
> grub-core/video/readers/tga.c | 7 +++
> include/grub/bitmap.h | 2 ++
> 5 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 0dbbdc374..2d6cd8358 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1515,7 +1515,8 @@ resolution. @xref{gfxmode}.
> Set a background image for use with the @samp{gfxterm} graphical terminal.
> The value of this option must be a file readable by GRUB at boot time, and
> it must end with @file{.png}, @file{.tga}, @file{.jpg}, or @file{.jpeg}.
> -The image will be scaled if necessary to fit the screen.
> +The image will be scaled if necessary to fit the screen. Image height and
> +width will be restricted by an artificial limit of 16384.
>
> @item GRUB_THEME
> Set a theme for use with the @samp{gfxterm} graphical terminal.
> diff --git a/grub-core/video/readers/jpeg.c b/grub-core/video/readers/jpeg.c
> index 09596fbf5..ae634fd41 100644
> --- a/grub-core/video/readers/jpeg.c
> +++ b/grub-core/video/readers/jpeg.c
> @@ -346,7 +346,11 @@ grub_jpeg_decode_sof (struct grub_jpeg_data *data)
>data->image_height = grub_jpeg_get_word (data);
>data->image_width = grub_jpeg_get_word (data);
>
> - if ((!data->image_height) || (!data->image_width))
> + grub_dprintf ("jpeg", "image height: %d\n", data->image_height);
> + grub_dprintf ("jpeg", "image width: %d\n", data->image_width);
> +
> + if ((!data->image_height) || (!data->image_width) ||
> + (data->image_height > IMAGE_HW_MAX_PX) || (data->image_width >
> IMAGE_HW_MAX_PX))
> return grub_error (GRUB_ERR_BAD_FILE_TYPE, "jpeg: invalid image size");
>
>cc = grub_jpeg_get_byte (data);
> diff --git a/grub-core/video/readers/png.c b/grub-core/video/readers/png.c
> index 7f2ba7849..3163e97bf 100644
> --- a/grub-core/video/readers/png.c
> +++ b/grub-core/video/readers/png.c
> @@ -264,7 +264,11 @@ grub_png_decode_image_header (struct grub_png_data *data)
>data->image_width = grub_png_get_dword (data);
>data->image_height = grub_png_get_dword (data);
>
> - if ((!data->image_height) || (!data->image_width))
> + grub_dprintf ("png", "image height: %d\n", data->image_height);
> + grub_dprintf ("png", "image width: %d\n", data->image_width);
> +
> + if ((!data->image_height) || (!data->image_width) ||
> + (data->image_height > IMAGE_HW_MAX_PX) || (data->image_width >
> IMAGE_HW_MAX_PX))
> return grub_error (GRUB_ERR_BAD_FILE_TYPE, "png: invalid image size");
>
>color_bits = grub_png_get_byte (data);
> diff --git a/grub-core/video/readers/tga.c b/grub-core/video/readers/tga.c
> index a9ec3a1b6..f2f563d06 100644
> --- a/grub-core/video/readers/tga.c
> +++ b/grub-core/video/readers/tga.c
> @@ -340,6 +340,13 @@ grub_video_reader_tga (struct grub_video_bitmap **bitmap,
>data.image_width = grub_le_to_cpu16 (data.hdr.image_width);
>data.image_height = grub_le_to_cpu16 (data.hdr.image_height);
>
> + grub_dprintf ("tga", "image height: %d\n", data.image_height);
> + grub_dprintf ("tga", "image width: %d\n", data.image_width);
> +
> + /* Check image height and width are within restrictions */
> + if ((data.image_height > IMAGE_HW_MAX_PX) || (data.image_width >
> IMAGE_HW_MAX_PX))
> +return grub_error (GRUB_ERR_BAD_FILE_TYPE, "tga: invalid image size");
> +
>/* Check that bitma
[PATCH v2 1/3] gnulib: Provide abort() implementation for gnulib
The recent gnulib updates require an implemention of abort(), but the
current macro provided by changeset:
cd37d3d3916c gnulib: Drop no-abort.patch
to config.h.in does not work with the clang compiler since it doesn't
provide a __builtin_trap implementation, so this element of the
changeset needs to be reverted, and replaced.
After some discussion with Vladimir 'phcoder' Serbinenko and Daniel
Kiper it was suggested to bring back in the change from the changeset:
db7337a3d353 "* grub-core/gnulib/regcomp.c (regerror): ..."
Which implements abort() as an inline call to grub_abort(), but since
that was made static by changeset:
a8f15bceeafe "* grub-core/kern/misc.c (grub_abort): Make static"
it is also necessary to revert the specific part that makes it a static
function too.
Another implementation of abort() was found in
grub-core/kern/compiler-rt.c which needs to also be removed to be
consistent.
Signed-off-by: Darren Kenny
---
config.h.in | 10 --
grub-core/kern/compiler-rt.c | 9 -
grub-core/kern/misc.c | 2 +-
grub-core/lib/posix_wrap/stdlib.h | 6 ++
include/grub/misc.h | 5 +
5 files changed, 8 insertions(+), 24 deletions(-)
diff --git a/config.h.in b/config.h.in
index 01dcbbfc82f0..4d1e50eba79c 100644
--- a/config.h.in
+++ b/config.h.in
@@ -137,16 +137,6 @@ typedef __UINT_FAST32_TYPE__ uint_fast32_t;
void * \
reallocarray (void *ptr, unsigned int nmemb, unsigned int size);
#define _GL_INLINE_HEADER_END _Pragma ("GCC diagnostic pop")
-
-/*
- * We don't have an abort() for gnulib to call in regexp. Because gnulib is
- * built as a separate object that is then linked with, it doesn't have any
- * of our headers or functions around to use - so we unfortunately can't
- * print anything. Builds of emu include the system's stdlib, which includes
- * a prototype for abort(), so leave this as a macro that doesn't take
- * arguments.
- */
-#define abort __builtin_trap
# endif /* !_GL_INLINE_HEADER_BEGIN */
/* gnulib doesn't build cleanly with older compilers. */
diff --git a/grub-core/kern/compiler-rt.c b/grub-core/kern/compiler-rt.c
index 8948fdf77278..8051552e3a1a 100644
--- a/grub-core/kern/compiler-rt.c
+++ b/grub-core/kern/compiler-rt.c
@@ -195,15 +195,6 @@ __ctzsi2 (grub_uint32_t x)
#endif
-#if defined (__clang__) && !defined(GRUB_EMBED_DECOMPRESSOR)
-/* clang emits references to abort(). */
-void __attribute__ ((noreturn))
-abort (void)
-{
- grub_fatal ("compiler abort");
-}
-#endif
-
#if (defined (__MINGW32__) || defined (__CYGWIN__))
void __register_frame_info (void)
{
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 6c0221cc336d..dfae4f9d7897 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -1249,7 +1249,7 @@ grub_printf_fmt_check (const char *fmt, const char
*fmt_expected)
/* Abort GRUB. This function does not return. */
-static void __attribute__ ((noreturn))
+void __attribute__ ((noreturn))
grub_abort (void)
{
grub_printf ("\nAborted.");
diff --git a/grub-core/lib/posix_wrap/stdlib.h
b/grub-core/lib/posix_wrap/stdlib.h
index 148e9d94bde0..f5279756abef 100644
--- a/grub-core/lib/posix_wrap/stdlib.h
+++ b/grub-core/lib/posix_wrap/stdlib.h
@@ -58,4 +58,10 @@ abs (int c)
return (c >= 0) ? c : -c;
}
+static inline void __attribute__ ((noreturn))
+abort (void)
+{
+ grub_abort ();
+}
+
#endif
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 892ac6a42dda..ddac3aae8bcc 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -385,6 +385,7 @@ char *EXPORT_FUNC(grub_xasprintf) (const char *fmt, ...)
__attribute__ ((format (GNU_PRINTF, 1, 2))) WARN_UNUSED_RESULT;
char *EXPORT_FUNC(grub_xvasprintf) (const char *fmt, va_list args)
WARN_UNUSED_RESULT;
void EXPORT_FUNC(grub_exit) (void) __attribute__ ((noreturn));
+void EXPORT_FUNC(grub_abort) (void) __attribute__ ((noreturn));
grub_uint64_t EXPORT_FUNC(grub_divmod64) (grub_uint64_t n,
grub_uint64_t d,
grub_uint64_t *r);
@@ -454,10 +455,6 @@ void EXPORT_FUNC(grub_reboot) (void) __attribute__
((noreturn));
void grub_reboot (void) __attribute__ ((noreturn));
#endif
-#if defined (__clang__) && !defined (GRUB_UTIL)
-void __attribute__ ((noreturn)) EXPORT_FUNC (abort) (void);
-#endif
-
#ifdef GRUB_MACHINE_PCBIOS
/* Halt the system, using APM if possible. If NO_APM is true, don't
* use APM even if it is available. */
--
2.31.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 3/3] build: Update to reflect minimum clang version 8.0
After doing some validation with clang from versions 3.8 and up, the builds prior to version 8.0.0 fail due to the use of safemath functions at link time. Signed-off-by: Darren Kenny fix Signed-off-by: Darren Kenny --- INSTALL | 2 +- include/grub/safemath.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/INSTALL b/INSTALL index 7bca64f69881..620dcceb4814 100644 --- a/INSTALL +++ b/INSTALL @@ -16,7 +16,7 @@ you don't have any of them, please obtain and install them before configuring the GRUB. * GCC 5.1.0 or later - Experimental support for clang 3.8.0 or later (results in much bigger binaries) + Experimental support for clang 8.0.0 or later (results in much bigger binaries) for i386, x86_64, arm (including thumb), arm64, mips(el), powerpc, sparc64 * GNU Make * GNU Bison 2.3 or later diff --git a/include/grub/safemath.h b/include/grub/safemath.h index c17b89bba17b..c2e338263107 100644 --- a/include/grub/safemath.h +++ b/include/grub/safemath.h @@ -23,15 +23,15 @@ #include -/* These appear in gcc 5.1 and clang 3.8. */ -#if GNUC_PREREQ(5, 1) || CLANG_PREREQ(3, 8) +/* These appear in gcc 5.1 and clang 8.0 */ +#if GNUC_PREREQ(5, 1) || CLANG_PREREQ(8, 0) #define grub_add(a, b, res)__builtin_add_overflow(a, b, res) #define grub_sub(a, b, res)__builtin_sub_overflow(a, b, res) #define grub_mul(a, b, res)__builtin_mul_overflow(a, b, res) #else -#error gcc 5.1 or newer or clang 3.8 or newer is required +#error gcc 5.1 or newer or clang 8.0 or newer is required #endif #endif /* GRUB_SAFEMATH_H */ -- 2.31.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 0/3] Fix building with clang
The abiltiy to build with clang was broken in the last release after the upgrade of gnulib, but it would also appear to have been broken too with versions of clang prior to 8.0.0. There were two main issues: - The use of __builtin_trap in the abort() macro. This builtin doesn't exist for clang builds After some discussion between Daniel and Vladimir, it was requested that I should revert some past changes in this area, and re-introduce the use of grub_abort(). - The is some use of variable length arrays (vla) in gnulib's code, and when an attempt was made to resolve this in gnulib itself, I was informed that we shouldn't be building gnulib with -Werror. Rather than totally disabling -Werror, it seemed better to just limit it for the specific case that is causing problems, i.e. vla. - Attempts to build clang with versions prior to 8.0.0 are also failing due to the use of the previously introduced safematch function usage. So we're also bumping the minimum version of clang in the INSTALL file and safemath.h where the test is done for the requisite version. Thanks, Darren. v1 -> v2 - Update with changes to INSTALL and safemath.h after testing various clang versions from 3.8 and up. Darren Kenny (3): gnulib: Provide abort() implementation for gnulib configure: Fix building with clang build: Update to reflect minimum clang version 8.0 INSTALL | 2 +- config.h.in | 10 -- configure.ac | 4 grub-core/kern/compiler-rt.c | 9 - grub-core/kern/misc.c | 2 +- grub-core/lib/posix_wrap/stdlib.h | 6 ++ include/grub/misc.h | 5 + include/grub/safemath.h | 6 +++--- 8 files changed, 16 insertions(+), 28 deletions(-) -- 2.31.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 2/3] configure: Fix building with clang
Building the current code with clang and the latest gnulib fails due to the use of a variable-length-array (vla) warning, which turns in to an error due to the presence of the -Werror during the build. The gnulib team stated that their code should not be built with -Werror. At present, the only way to do this is for the complete code-base, by using the --disable-werror option to configure. Rather than doing this, and failing to gain any benefit that it provides, instead, if building with clang, this patch makes it possible to specifically not error on vlas, while retaining the -Werror functionality otherwise. Signed-off-by: Darren Kenny --- configure.ac | 4 1 file changed, 4 insertions(+) diff --git a/configure.ac b/configure.ac index 1348b06a985a..93626b7982d4 100644 --- a/configure.ac +++ b/configure.ac @@ -1939,6 +1939,10 @@ AC_ARG_ENABLE([werror], if test x"$enable_werror" != xno ; then TARGET_CFLAGS="$TARGET_CFLAGS -Werror" HOST_CFLAGS="$HOST_CFLAGS -Werror" + if test "x$grub_cv_cc_target_clang" = xyes; then +TARGET_CFLAGS="$TARGET_CFLAGS -Wno-error=vla" +HOST_CFLAGS="$HOST_CFLAGS -Wno-error=vla" + fi fi TARGET_CPP="$TARGET_CC -E" -- 2.31.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/2] Fix building with clang
The abiltiy to build with clang was broken in the last release after the upgrade of gnulib. There were two main issues: - The use of __builtin_trap in the abort() macro. This builtin doesn't exist for clang builds After some discussion between Daniel and Vladimir, it was requested that I should revert some past changes in this area, and re-introduce the use of grub_abort(). - The is some use of variable length arrays (vla) in gnulib's code, and when an attempt was made to resolve this in gnulib itself, I was informed that we shouldn't be building gnulib with -Werror. Rather than totally disabling -Werror, it seemed better to just limit it for the specific case that is causing problems, i.e. vla. Thanks, Darren. Darren Kenny (2): gnulib: Provide abort() implementation for gnulib configure: Fix building with clang config.h.in | 10 -- configure.ac | 4 grub-core/kern/misc.c | 2 +- grub-core/lib/posix_wrap/stdlib.h | 6 ++ include/grub/misc.h | 5 + 5 files changed, 12 insertions(+), 15 deletions(-) -- 2.31.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/2] gnulib: Provide abort() implementation for gnulib
The recent gnulib updates require an implemention of abort(), but the
current macro provided by changeset:
cd37d3d3916c gnulib: Drop no-abort.patch
to config.h.in does not work with the clang compiler since it doesn't
provide a __builtin_trap implementation, so this element of the
changeset needs to be reverted, and replaced.
After some discussion with Vladimir 'phcoder' Serbinenko and Daniel
Kiper it was suggested to bring back in the change from the changeset:
db7337a3d353 "* grub-core/gnulib/regcomp.c (regerror): ..."
Which implements abort() as an inline call to grub_abort(), but since
that was made static by changeset:
a8f15bceeafe "* grub-core/kern/misc.c (grub_abort): Make static"
it is also necessary to revert the specific part that makes it a static
function too.
Signed-off-by: Darren Kenny
---
config.h.in | 10 --
grub-core/kern/misc.c | 2 +-
grub-core/lib/posix_wrap/stdlib.h | 6 ++
include/grub/misc.h | 5 +
4 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/config.h.in b/config.h.in
index 01dcbbfc82f0..4d1e50eba79c 100644
--- a/config.h.in
+++ b/config.h.in
@@ -137,16 +137,6 @@ typedef __UINT_FAST32_TYPE__ uint_fast32_t;
void * \
reallocarray (void *ptr, unsigned int nmemb, unsigned int size);
#define _GL_INLINE_HEADER_END _Pragma ("GCC diagnostic pop")
-
-/*
- * We don't have an abort() for gnulib to call in regexp. Because gnulib is
- * built as a separate object that is then linked with, it doesn't have any
- * of our headers or functions around to use - so we unfortunately can't
- * print anything. Builds of emu include the system's stdlib, which includes
- * a prototype for abort(), so leave this as a macro that doesn't take
- * arguments.
- */
-#define abort __builtin_trap
# endif /* !_GL_INLINE_HEADER_BEGIN */
/* gnulib doesn't build cleanly with older compilers. */
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 6c0221cc336d..dfae4f9d7897 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -1249,7 +1249,7 @@ grub_printf_fmt_check (const char *fmt, const char
*fmt_expected)
/* Abort GRUB. This function does not return. */
-static void __attribute__ ((noreturn))
+void __attribute__ ((noreturn))
grub_abort (void)
{
grub_printf ("\nAborted.");
diff --git a/grub-core/lib/posix_wrap/stdlib.h
b/grub-core/lib/posix_wrap/stdlib.h
index 148e9d94bde0..f5279756abef 100644
--- a/grub-core/lib/posix_wrap/stdlib.h
+++ b/grub-core/lib/posix_wrap/stdlib.h
@@ -58,4 +58,10 @@ abs (int c)
return (c >= 0) ? c : -c;
}
+static inline void __attribute__ ((noreturn))
+abort (void)
+{
+ grub_abort ();
+}
+
#endif
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 892ac6a42dda..ddac3aae8bcc 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -385,6 +385,7 @@ char *EXPORT_FUNC(grub_xasprintf) (const char *fmt, ...)
__attribute__ ((format (GNU_PRINTF, 1, 2))) WARN_UNUSED_RESULT;
char *EXPORT_FUNC(grub_xvasprintf) (const char *fmt, va_list args)
WARN_UNUSED_RESULT;
void EXPORT_FUNC(grub_exit) (void) __attribute__ ((noreturn));
+void EXPORT_FUNC(grub_abort) (void) __attribute__ ((noreturn));
grub_uint64_t EXPORT_FUNC(grub_divmod64) (grub_uint64_t n,
grub_uint64_t d,
grub_uint64_t *r);
@@ -454,10 +455,6 @@ void EXPORT_FUNC(grub_reboot) (void) __attribute__
((noreturn));
void grub_reboot (void) __attribute__ ((noreturn));
#endif
-#if defined (__clang__) && !defined (GRUB_UTIL)
-void __attribute__ ((noreturn)) EXPORT_FUNC (abort) (void);
-#endif
-
#ifdef GRUB_MACHINE_PCBIOS
/* Halt the system, using APM if possible. If NO_APM is true, don't
* use APM even if it is available. */
--
2.31.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/2] configure: Fix building with clang
Building the current code with clang and the latest gnulib fails due to the use of a variable-length-array (vla) warning, which turns in to an error due to the presence of the -Werror during the build. The gnulib team stated that their code should not be built with -Werror. At present, the only way to do this is for the complete code-base, by using the --disable-werror option to configure. Rather than doing this, and failing to gain any benefit that it provides, instead, if building with clang, this patch makes it possible to specifically not error on vlas, while retaining the -Werror functionality otherwise. Signed-off-by: Darren Kenny --- configure.ac | 4 1 file changed, 4 insertions(+) diff --git a/configure.ac b/configure.ac index 1348b06a985a..93626b7982d4 100644 --- a/configure.ac +++ b/configure.ac @@ -1939,6 +1939,10 @@ AC_ARG_ENABLE([werror], if test x"$enable_werror" != xno ; then TARGET_CFLAGS="$TARGET_CFLAGS -Werror" HOST_CFLAGS="$HOST_CFLAGS -Werror" + if test "x$grub_cv_cc_target_clang" = xyes; then +TARGET_CFLAGS="$TARGET_CFLAGS -Wno-error=vla" +HOST_CFLAGS="$HOST_CFLAGS -Wno-error=vla" + fi fi TARGET_CPP="$TARGET_CC -E" -- 2.31.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] grub-core/disk/cryptodisk.c: Fix unintentional integer overflow
Hi Alec,
This looks good, thanks for fixing it.
On Thursday, 2022-10-13 at 22:13:44 +01, Alec Brown wrote:
> In the function grub_cryptodisk_endecrypt(), a for loop is incrementing the
> variable i by (1U << log_sector_size). The variable i is of type grub_size_t
> which is a 64-bit unsigned integer on x86_64 architecture. On the other hand,
> 1U
> is a 32-bit unsigned integer. By performing a left shift between these two
> values of different types, there may be potential for an integer overflow. To
> avoid this, we replace 1U with (grub_size_t) 1.
>
> Fixes: CID 307788
>
> Signed-off-by: Alec Brown
Reviewed-by: Darren Kenny
Thanks,
Darren.
> ---
> grub-core/disk/cryptodisk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 9f5dc7acb..cdcb882ca 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -239,7 +239,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> return (do_encrypt ? grub_crypto_ecb_encrypt (dev->cipher, data, data,
> len)
> : grub_crypto_ecb_decrypt (dev->cipher, data, data, len));
>
> - for (i = 0; i < len; i += (1U << log_sector_size))
> + for (i = 0; i < len; i += ((grub_size_t) 1 << log_sector_size))
> {
>grub_size_t sz = ((dev->cipher->cipher->blocksize
>+ sizeof (grub_uint32_t) - 1)
> --
> 2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] fill_fs_info: pass pointer to dnode_end_t instead of value
Hi Jag,
These changes look good to me.
Just to confirm, you have run 'make check' and this has no negative
impact on the zfs tests?
Assuming that is the case...
On Friday, 2022-08-19 at 10:57:22 -04, Jagannathan Raman wrote:
> Coverity reports that dnode_end_t argument of fill_fs_info() is too
> large to pass-by-value. Therefore, replace the argument with a pointer.
>
> Fixes: CID 73631
>
> Signed-off-by: Jagannathan Raman
>
Reviewed-by: Darren Kenny
Thanks,
Darren.
> ---
> grub-core/fs/zfs/zfs.c | 17 +
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> index ffa0e5863..975c67242 100644
> --- a/grub-core/fs/zfs/zfs.c
> +++ b/grub-core/fs/zfs/zfs.c
> @@ -3983,14 +3983,23 @@ grub_zfs_getmdnobj (grub_device_t dev, const char
> *fsfilename,
>return err;
> }
>
> +/*
> + * Note: fill_fs_info() uses functions such as make_mdn() that modify
> + * the input dnode_end_t parameter. However, we should not allow it.
> + * Therefore, we are making mdn_in constant - fill_fs_info() makes a
> + * local copy of it.
> + */
> static grub_err_t
> fill_fs_info (struct grub_dirhook_info *info,
> - dnode_end_t mdn, struct grub_zfs_data *data)
> + const dnode_end_t *mdn_in, struct grub_zfs_data *data)
> {
>grub_err_t err;
>dnode_end_t dn;
>grub_uint64_t objnum;
>grub_uint64_t headobj;
> + dnode_end_t mdn;
> +
> + grub_memcpy (, mdn_in, sizeof (*mdn_in));
>
>grub_memset (info, 0, sizeof (*info));
>
> @@ -4148,7 +4157,7 @@ iterate_zap_fs (const char *name, grub_uint64_t val,
>if (mdn.dn.dn_type != DMU_OT_DSL_DIR)
> return 0;
>
> - err = fill_fs_info (, mdn, ctx->data);
> + err = fill_fs_info (, , ctx->data);
>if (err)
> {
>grub_errno = 0;
> @@ -4179,7 +4188,7 @@ iterate_zap_snap (const char *name, grub_uint64_t val,
>if (mdn.dn.dn_type != DMU_OT_DSL_DATASET)
> return 0;
>
> - err = fill_fs_info (, mdn, ctx->data);
> + err = fill_fs_info (, , ctx->data);
>if (err)
> {
>grub_errno = 0;
> @@ -4224,7 +4233,7 @@ grub_zfs_dir (grub_device_t device, const char *path,
>dnode_end_t dn;
>struct grub_dirhook_info info;
>
> - err = fill_fs_info (, data->dnode, data);
> + err = fill_fs_info (, >dnode, data);
>if (err)
> {
> zfs_unmount (data);
> --
> 2.20.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 0/5] Fix coverity bugs and add checks for elf values in grub-core
Hi Alec,
These changes look good to me,
Reviewed-by: Darren Kenny
Thanks,
Darren.
On Friday, 2022-08-12 at 18:25:44 -04, Alec Brown wrote:
> Updates from v1:
> - A few patches didn't work on 32-bit platforms. Fixed this by renaming
> function definitions in multiboot_elfxx.c. The patches that did work
> were
> merged with the master branch.
> - Added a patch to make error conditionals consistent in comparing to
> GRUB_ERR_NONE.
>
> Coverity identified several untrusted loop bounds and untrusted allocation
> size
> bugs in grub-core/loader/i386/bsdXX.c and grub-core/loader/multiboot_elfXX.c.
> Upon review of these bugs, I found that specific checks weren't being made to
> various elf header values based on the elf manual page. The first version of
> the patch series contained the patches fixing the Coverity bugs, but those
> have
> been merged with the master branch. The remaining patches add functions to
> check for the correct elf header values, update previous work done in
> util/grub-module-verifierXX.c that checks elf header values, and update error
> conditionals of the affected files.
>
> Also, I was able to verify that these patches work by using Multiboot and
> Multiboot2 to boot a Xen image.
>
> Alec Brown (5):
> elf: Validate number of elf section header table entries
> elf: Validate elf section header table index for section name string
> table
> elf: Validate number of elf program header table entries
> util/grub-module-verifierXX.c: Changed get_shnum() return type
> loader: Update error conditionals to use enums
>
> grub-core/kern/elf.c | 18 ++
> grub-core/kern/elfXX.c | 101
> +
> grub-core/loader/i386/bsdXX.c | 131
> +++
> grub-core/loader/multiboot_elfxx.c | 85
> +
> include/grub/elf.h | 23 +++
> util/grub-module-verifierXX.c | 10 ++
> 6 files changed, 292 insertions(+), 76 deletions(-)
>
> Interdiff against v1:
> diff --git a/grub-core/loader/i386/bsdXX.c b/grub-core/loader/i386/bsdXX.c
> index 2cc1daa49..09d909f1b 100644
> --- a/grub-core/loader/i386/bsdXX.c
> +++ b/grub-core/loader/i386/bsdXX.c
> @@ -52,7 +52,7 @@ read_headers (grub_file_t file, const char *filename,
> Elf_Ehdr *e, Elf_Shdr **sh
> return grub_error (GRUB_ERR_BAD_OS, N_("invalid arch-dependent ELF
> magic"));
>
>err = grub_elf_get_shnum (e, );
> - if (err)
> + if (err != GRUB_ERR_NONE)
> return err;
>
>*shdr = grub_calloc (shnum, e->e_shentsize);
> @@ -94,7 +94,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct
> grub_relocator *relocator,
>curload = module = ALIGN_PAGE (*kern_end);
>
>err = read_headers (file, argv[0], , );
> - if (err)
> + if (err != GRUB_ERR_NONE)
> goto out;
>
>err = grub_elf_get_shnum (, );
> @@ -118,7 +118,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct
> grub_relocator *relocator,
> grub_relocator_chunk_t ch;
> err = grub_relocator_alloc_chunk_addr (relocator, ,
>module, chunk_size);
> -if (err)
> +if (err != GRUB_ERR_NONE)
>goto out;
> chunk_src = get_virtual_current_address (ch);
>}
> @@ -143,7 +143,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct
> grub_relocator *relocator,
> case SHT_PROGBITS:
> err = load (file, argv[0], (grub_uint8_t *) chunk_src + curload -
> *kern_end,
> s->sh_offset, s->sh_size);
> - if (err)
> + if (err != GRUB_ERR_NONE)
> goto out;
> break;
> case SHT_NOBITS:
> @@ -159,11 +159,11 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct
> grub_relocator *relocator,
>err = grub_freebsd_add_meta_module (argv[0],
> FREEBSD_MODTYPE_ELF_MODULE_OBJ,
> argc - 1, argv + 1, module,
> curload - module);
> - if (! err)
> + if (err == GRUB_ERR_NONE)
> err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA
> | FREEBSD_MODINFOMD_ELFHDR,
> , sizeof (e));
> - if (! err)
> + if (err == GRUB_ERR_NONE)
> err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA
> | FREEBSD_MODINFOMD_
Re: [PATCH 2/2] util: confirm directory creation in grub_install_mkdir_p
Hi Vladimir,
I admit, that I did not previously.
I just tried it out where one of the components in a deeper path was a
symlink, and it works as I would expect. It will see it as a
directory in grub_util_is_directory() using stat(), since that is
looking at what is pointed to, in comparison to lstat(), which only
looks at the symlink itself.
Thanks,
Darren.
On Tuesday, 2022-08-09 at 16:04:32 +02, Vladimir Serbinenko wrote:
> Did you test the case when some of components exist and are symlinks? E.g.
> /temp being a symlinkto /var/tmp
>
> Le mar. 9 août 2022, 15:30, Darren Kenny a écrit :
>
>> Because grub_util_mkdir() is implemented to not return a value on any
>> platform, grub_instal_mkdir_p can test for success by confirming that
>> the directory requested exists after attempting to create it, otherwise
>> it should fail with an error and exit.
>>
>> While fixing this, a flaw in the logic was shown, where the first match
>> of the path separator, which almost always was the first character in
>> the path (e.g. /boot/grub2) would result in creating a directory with an
>> empty name (i.e. ""). To avoid that, it should skip the handling of the
>> path separator where p is pointing to the first character.
>>
>> Signed-off-by: Darren Kenny
>> ---
>> util/grub-install-common.c | 7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
>> index 347558bf5412..035293c2357e 100644
>> --- a/util/grub-install-common.c
>> +++ b/util/grub-install-common.c
>> @@ -173,15 +173,20 @@ grub_install_mkdir_p (const char *dst)
>>char *p;
>>for (p = t; *p; p++)
>> {
>> - if (is_path_separator (*p))
>> + if (is_path_separator (*p) && p != t)
>> {
>> char s = *p;
>> *p = '\0';
>> grub_util_mkdir (t);
>> + if (!grub_util_is_directory(t))
>> + grub_util_error (_("failed to make directory: '%s'"), t);
>> +
>> *p = s;
>> }
>> }
>>grub_util_mkdir (t);
>> + if (!grub_util_is_directory(t))
>> +grub_util_error (_("failed to make directory: '%s'"), t);
>>free (t);
>> }
>>
>> --
>> 2.31.1
>>
>>
>> ___
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/2] util: Ignore return value for grub_util_mkdir() on all platforms
Coverity signaled 2 issues where the return value of grub_util_mkdir()
was not being tested.
The Windows variant of this code defines the function as having no
return value (void), but the UNIX variants all are mapped using a macro
to the libc mkdir() function, which returns an int value.
To be consistent, the mapping should cast to void to for these too.
Fixes: CID 73583
Fixes: CID 73617
Signed-off-by: Darren Kenny
---
include/grub/osdep/hostfile_aros.h | 2 +-
include/grub/osdep/hostfile_unix.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/grub/osdep/hostfile_aros.h
b/include/grub/osdep/hostfile_aros.h
index 161fbb7bdfd6..bd2c62c9a716 100644
--- a/include/grub/osdep/hostfile_aros.h
+++ b/include/grub/osdep/hostfile_aros.h
@@ -74,7 +74,7 @@ grub_util_readlink (const char *name, char *buf, size_t
bufsize)
return readlink(name, buf, bufsize);
}
-#define grub_util_mkdir(a) mkdir ((a), 0755)
+#define grub_util_mkdir(a) (void)mkdir ((a), 0755)
struct grub_util_fd
{
diff --git a/include/grub/osdep/hostfile_unix.h
b/include/grub/osdep/hostfile_unix.h
index 17cd3aa8b304..e6f082f259cb 100644
--- a/include/grub/osdep/hostfile_unix.h
+++ b/include/grub/osdep/hostfile_unix.h
@@ -77,7 +77,7 @@ grub_util_readlink (const char *name, char *buf, size_t
bufsize)
return readlink(name, buf, bufsize);
}
-#define grub_util_mkdir(a) mkdir ((a), 0755)
+#define grub_util_mkdir(a) (void)mkdir ((a), 0755)
#if defined (__NetBSD__)
/* NetBSD uses /boot for its boot block. */
--
2.31.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/2] util: Resolve issues with use of grub_util_mkdir
Coverity flagged some issues where grub_util_mkdir() was being called but the
return value was not being checked.
After examining the implementation, on UNIX and ACROS, grub_util_mkdir() is a
macro that maps to libc's mkdir(), which returns an int.
But on MS Windows, grub_util_mkdir() is a function with a void return, and
which in turn eventually maps to the MS API CreateDirectory() call.
After discussing with Daniel, it was felt that fixing this by reimplementing
the Windows code for grub_util_mkdir() to return something like the
libc equivalents. This and then testing the return values on each call to
grub_util_mkdir() in the util code is of little benefit when the main usage is
to populate a temporary directory that was created earlier in the code of
grub-mkrescue, which should never fail.
The solution presented in patch 1, to resolve Coverity's concerns is to cast
the return value of the mkdir() command to void in the macro for
grub_util_mkdir().
The other use-case is in the grub_util_mkdir_p() function that creates
a directory tree, as the UNIX command mkdir -p does. This code should at least
try confirm that it is successfully creating the parent directories before it
continues to the sub-directories. This is done by using
grub_util_is_directory() to ensure that the directory exists, and if it
doesn't then to call grub_util_error() with some information and exit.
In fixing this, it was discovered that grub_util_mkdir() was incorrectly being
called with an empty directory name (""), which was due to an error in the
tokenisation being performed.
Darren Kenny (2):
util: Ignore return value for grub_util_mkdir() on all platforms
util: confirm directory creation in grub_install_mkdir_p
include/grub/osdep/hostfile_aros.h | 2 +-
include/grub/osdep/hostfile_unix.h | 2 +-
util/grub-install-common.c | 7 ++-
3 files changed, 8 insertions(+), 3 deletions(-)
--
2.31.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/2] util: confirm directory creation in grub_install_mkdir_p
Because grub_util_mkdir() is implemented to not return a value on any
platform, grub_instal_mkdir_p can test for success by confirming that
the directory requested exists after attempting to create it, otherwise
it should fail with an error and exit.
While fixing this, a flaw in the logic was shown, where the first match
of the path separator, which almost always was the first character in
the path (e.g. /boot/grub2) would result in creating a directory with an
empty name (i.e. ""). To avoid that, it should skip the handling of the
path separator where p is pointing to the first character.
Signed-off-by: Darren Kenny
---
util/grub-install-common.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/util/grub-install-common.c b/util/grub-install-common.c
index 347558bf5412..035293c2357e 100644
--- a/util/grub-install-common.c
+++ b/util/grub-install-common.c
@@ -173,15 +173,20 @@ grub_install_mkdir_p (const char *dst)
char *p;
for (p = t; *p; p++)
{
- if (is_path_separator (*p))
+ if (is_path_separator (*p) && p != t)
{
char s = *p;
*p = '\0';
grub_util_mkdir (t);
+ if (!grub_util_is_directory(t))
+ grub_util_error (_("failed to make directory: '%s'"), t);
+
*p = s;
}
}
grub_util_mkdir (t);
+ if (!grub_util_is_directory(t))
+grub_util_error (_("failed to make directory: '%s'"), t);
free (t);
}
--
2.31.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/1] gensymlist: fix clang build with exporting of __builtin_trap
Hi Glenn,
Finally managed to get a look at this again, and I agree Glenn that your
patch fixes this issue too, so I withdraw my patch.
Thanks,
Darren.
On Tuesday, 2022-06-14 at 13:18:43 -05, Glenn Washburn wrote:
> On Tue, 14 Jun 2022 18:55:18 +0200
> "Vladimir 'phcoder' Serbinenko" wrote:
>
>> Correct solution is to provide __builtin_trap ourselves. Likely it would be
>> a wrapper around grub_abort
>
> This seems like tacit support for at least the direction of my patch
> removing the use of __builtin_trap[1] which was introduced recently in
> cd37d3d39 (gnulib: Drop no-abort.patch) and which uses grub_abort()
> instead of __builtin_trap. The only other place that could potentially
> use __builtin_trap is in grub-core/lib/gnulib/verify.h in the "assume"
> macro. However, I don't see anywhere that macro is being used. Though
> Daniel was wondering what was wrong with defining abort in
> grub-core/lib/posix_wrap/stdlib.h as had been done in db7337a3d
> (grub-core/lib/posix_wrap/stdlib.h (abort): Removed).
>
> Glenn
>
> [1] https://lists.gnu.org/archive/html/grub-devel/2022-03/msg00220.html
>
>>
>> Le mar. 14 juin 2022, 15:37, Darren Kenny a
>> écrit :
>>
>> > clang expands the abort function to __builtin_trap, but that cannot be
>> > exported.
>> >
>> > The script that generates the symlist.c file should also exclude any
>> > symbols that start with __builtin_.
>> >
>> > Signed-off-by: Darren Kenny
>> > ---
>> > grub-core/gensymlist.sh | 1 +
>> > 1 file changed, 1 insertion(+)
>> >
>> > diff --git a/grub-core/gensymlist.sh b/grub-core/gensymlist.sh
>> > index 5074ef6aad58..a2e5b85d0a71 100644
>> > --- a/grub-core/gensymlist.sh
>> > +++ b/grub-core/gensymlist.sh
>> > @@ -58,6 +58,7 @@ EOF
>> >
>> > (while read LINE; do echo $LINE; done) \
>> >| grep -v '^#' \
>> > + | grep -v 'EXPORT_FUNC *(__builtin_[a-zA-Z0-9_]*)' \
>> >| sed -n \
>> > -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC
>> > *(\([a-zA-Z0-9_]*\)).*/ {"\1", \1, 1},/;p;}' \
>> > -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR
>> > *(\([a-zA-Z0-9_]*\)).*/ {"\1", (void *) \&\1, 0},/;p;}' \
>> > --
>> > 2.31.1
>> >
>> >
>> > ___
>> > Grub-devel mailing list
>> > Grub-devel@gnu.org
>> > https://lists.gnu.org/mailman/listinfo/grub-devel
>> >
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Initialize local relocator subchunk struct to all zeros
Hi Ross,
This looks good to me.
On Thursday, 2022-07-14 at 09:41:28 -04, Ross Philipson wrote:
> The way the code is written the tofree variable would never be
> passed to the free_subchunk() function uninitialized. Coverity
> cannot determine this and flags the situation as "Using uninitialized
> value...". The fix is just to initialize the local struct.
>
> Fixes: CID 314016
>
> Signed-off-by: Ross Philipson
Reviewed-by: Darren Kenny
Thanks,
Darren.
> ---
> grub-core/lib/relocator.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/lib/relocator.c b/grub-core/lib/relocator.c
> index 68ef128..bfcc70d 100644
> --- a/grub-core/lib/relocator.c
> +++ b/grub-core/lib/relocator.c
> @@ -989,7 +989,7 @@ malloc_in_range (struct grub_relocator *rel,
> if (j != 0 && events[j - 1].pos != events[j].pos)
> {
> grub_addr_t alloc_start, alloc_end;
> - struct grub_relocator_subchunk tofree;
> + struct grub_relocator_subchunk tofree = {0};
> struct grub_relocator_subchunk *curschu =
> if (!oom)
> curschu = >subchunks[cural];
> --
> 1.8.3.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v3 1/1] mkfont: Fix tainted loop boundary issues with substitutions
With gsub substitutions the offsets should be validated against the
the number of glyphs in a font face and the memory allocated for the gsub
substitution data.
Both the number of glyphs and the last address in the allocated data are
passed in to process_cursive(), where the number of glyphs validates the end
of the range.
Enabling memory allocation validation uses two macros, one to simply check the
address against the allocated space, and the other to check that the number of
items of a given size doesn't extend outside of the allocated space.
Fixes: CID 73770
Fixes: CID 314040
Signed-off-by: Darren Kenny
---
v2 -> v3:
Updated to resolve style issues around the spacing when calling macros.
util/grub-mkfont.c | 62 +++---
1 file changed, 53 insertions(+), 9 deletions(-)
diff --git a/util/grub-mkfont.c b/util/grub-mkfont.c
index d0e5ec564f03..5f4130951eb1 100644
--- a/util/grub-mkfont.c
+++ b/util/grub-mkfont.c
@@ -67,6 +67,11 @@
#define GRUB_FONT_RANGE_BLOCK 1024
+#define GSUB_PTR_VALID(x, end) assert((grub_uint8_t*)(x) <= (end))
+
+#define GSUB_ARRAY_SIZE_VALID(a, sz, end) \
+assert((sz) >= 0 && ((sz) <= ((end) - (grub_uint8_t*)(a)) / sizeof(*(a
+
struct grub_glyph_info
{
struct grub_glyph_info *next;
@@ -487,16 +492,22 @@ subst (const struct gsub_substitution *sub, grub_uint32_t
glyph,
static void
process_cursive (struct gsub_feature *feature,
struct gsub_lookup_list *lookups,
-grub_uint32_t feattag)
+grub_uint32_t feattag,
+grub_uint32_t num_glyphs,
+grub_uint8_t *gsub_end)
{
int j, k;
int i;
+ int lookup_count = grub_be_to_cpu16 (feature->lookupcount);
struct glyph_replace **target = NULL;
struct gsub_substitution *sub;
- for (j = 0; j < grub_be_to_cpu16 (feature->lookupcount); j++)
+ GSUB_ARRAY_SIZE_VALID (feature->lookupindices, lookup_count, gsub_end);
+
+ for (j = 0; j < lookup_count; j++)
{
int lookup_index = grub_be_to_cpu16 (feature->lookupindices[j]);
+ int sub_count;
struct gsub_lookup *lookup;
if (lookup_index >= grub_be_to_cpu16 (lookups->count))
{
@@ -538,7 +549,12 @@ process_cursive (struct gsub_feature *feature,
target = _medijoin;
break;
}
- for (k = 0; k < grub_be_to_cpu16 (lookup->subtablecount); k++)
+
+ sub_count = grub_be_to_cpu16 (lookup->subtablecount);
+
+ GSUB_ARRAY_SIZE_VALID (lookup->subtables, sub_count, gsub_end);
+
+ for (k = 0; k < sub_count; k++)
{
sub = (struct gsub_substitution *)
((grub_uint8_t *) lookup + grub_be_to_cpu16 (lookup->subtables[k]));
@@ -559,18 +575,33 @@ process_cursive (struct gsub_feature *feature,
if (covertype == GSUB_COVERAGE_LIST)
{
struct gsub_coverage_list *cover = coverage;
+ int count = grub_be_to_cpu16 (cover->count);
int l;
- for (l = 0; l < grub_be_to_cpu16 (cover->count); l++)
+
+ GSUB_ARRAY_SIZE_VALID (cover->glyphs, count, gsub_end);
+
+ for (l = 0; l < count; l++)
subst (sub, grub_be_to_cpu16 (cover->glyphs[l]), target, );
}
else if (covertype == GSUB_COVERAGE_RANGE)
{
struct gsub_coverage_ranges *cover = coverage;
+ int count = grub_be_to_cpu16 (cover->count);
int l, m;
- for (l = 0; l < grub_be_to_cpu16 (cover->count); l++)
- for (m = grub_be_to_cpu16 (cover->ranges[l].start);
-m <= grub_be_to_cpu16 (cover->ranges[l].end); m++)
- subst (sub, m, target, );
+
+ GSUB_ARRAY_SIZE_VALID (cover->ranges, count, gsub_end);
+
+ for (l = 0; l < count; l++)
+ {
+ grub_uint16_t start = grub_be_to_cpu16
(cover->ranges[l].start);
+ grub_uint16_t end = grub_be_to_cpu16 (cover->ranges[l].end);
+
+ if (start > end || end > num_glyphs)
+ grub_util_error ("%s", _("invalid font range"));
+
+ for (m = start; m <= end; m++)
+ subst (sub, m, target, );
+ }
}
else
/* TRANSLATORS: most font transformations apply only to
@@ -589,6 +620,7 @@ add_font (struct grub_font_info *font_info, FT_Face face,
int nocut)
{
struct gsub_header *gsub = NULL;
FT_ULong gsub_len = 0;
+ grub_uint8_t *gsub_end = NULL;
if (!FT_Load_Sfnt_Table (face, TTAG_GSUB, 0, NULL, _len))
{
@@ -600,6 +632,9 @@ add_font (struct grub_font_info *font_info, FT_Face face,
int nocut)
gsub_len = 0;
}
}
+
+ gsub_end = (grub_uint8_t *) gsub + gsub_len;
+
if (g
[PATCH v2 1/1] mkfont: Fix tainted loop boundary issues with substitutions
With gsub substitutions the offsets should be validated against the
the number of glyphs in a font face and the memory allocated for the gsub
substitution data.
Both the number of glyphs and the last address in the allocated data are
passed in to process_cursive(), where the number of glyphs validates the end
of the range.
Enabling memory allocation validation uses two macros, one to simply check the
address against the allocated space, and the other to check that the number of
items of a given size doesn't extend outside of the allocated space.
Fixes: CID 73770
Fixes: CID 314040
Signed-off-by: Darren Kenny
---
[Fixes issue in v1 where had declarations out of order]
util/grub-mkfont.c | 62 +++---
1 file changed, 53 insertions(+), 9 deletions(-)
diff --git a/util/grub-mkfont.c b/util/grub-mkfont.c
index d0e5ec564f03..90538788e0e9 100644
--- a/util/grub-mkfont.c
+++ b/util/grub-mkfont.c
@@ -67,6 +67,11 @@
#define GRUB_FONT_RANGE_BLOCK 1024
+#define GSUB_PTR_VALID(x, end) assert((grub_uint8_t*)(x) <= end)
+
+#define GSUB_ARRAY_SIZE_VALID(a, sz, end) \
+assert((sz) >= 0 && ((sz) <= (end - (grub_uint8_t*)(a)) / sizeof(*(a
+
struct grub_glyph_info
{
struct grub_glyph_info *next;
@@ -487,16 +492,22 @@ subst (const struct gsub_substitution *sub, grub_uint32_t
glyph,
static void
process_cursive (struct gsub_feature *feature,
struct gsub_lookup_list *lookups,
-grub_uint32_t feattag)
+grub_uint32_t feattag,
+grub_uint32_t num_glyphs,
+grub_uint8_t *gsub_end)
{
int j, k;
int i;
+ int lookup_count = grub_be_to_cpu16 (feature->lookupcount);
struct glyph_replace **target = NULL;
struct gsub_substitution *sub;
- for (j = 0; j < grub_be_to_cpu16 (feature->lookupcount); j++)
+ GSUB_ARRAY_SIZE_VALID(feature->lookupindices, lookup_count, gsub_end);
+
+ for (j = 0; j < lookup_count; j++)
{
int lookup_index = grub_be_to_cpu16 (feature->lookupindices[j]);
+ int sub_count;
struct gsub_lookup *lookup;
if (lookup_index >= grub_be_to_cpu16 (lookups->count))
{
@@ -538,7 +549,12 @@ process_cursive (struct gsub_feature *feature,
target = _medijoin;
break;
}
- for (k = 0; k < grub_be_to_cpu16 (lookup->subtablecount); k++)
+
+ sub_count = grub_be_to_cpu16 (lookup->subtablecount);
+
+ GSUB_ARRAY_SIZE_VALID(lookup->subtables, sub_count, gsub_end);
+
+ for (k = 0; k < sub_count; k++)
{
sub = (struct gsub_substitution *)
((grub_uint8_t *) lookup + grub_be_to_cpu16 (lookup->subtables[k]));
@@ -559,18 +575,33 @@ process_cursive (struct gsub_feature *feature,
if (covertype == GSUB_COVERAGE_LIST)
{
struct gsub_coverage_list *cover = coverage;
+ int count = grub_be_to_cpu16 (cover->count);
int l;
- for (l = 0; l < grub_be_to_cpu16 (cover->count); l++)
+
+ GSUB_ARRAY_SIZE_VALID(cover->glyphs, count, gsub_end);
+
+ for (l = 0; l < count; l++)
subst (sub, grub_be_to_cpu16 (cover->glyphs[l]), target, );
}
else if (covertype == GSUB_COVERAGE_RANGE)
{
struct gsub_coverage_ranges *cover = coverage;
+ int count = grub_be_to_cpu16 (cover->count);
int l, m;
- for (l = 0; l < grub_be_to_cpu16 (cover->count); l++)
- for (m = grub_be_to_cpu16 (cover->ranges[l].start);
-m <= grub_be_to_cpu16 (cover->ranges[l].end); m++)
- subst (sub, m, target, );
+
+ GSUB_ARRAY_SIZE_VALID(cover->ranges, count, gsub_end);
+
+ for (l = 0; l < count; l++)
+ {
+ grub_uint16_t start = grub_be_to_cpu16
(cover->ranges[l].start);
+ grub_uint16_t end = grub_be_to_cpu16 (cover->ranges[l].end);
+
+ if (start > end || end > num_glyphs)
+ grub_util_error ("%s", _("invalid font range"));
+
+ for (m = start; m <= end; m++)
+ subst (sub, m, target, );
+ }
}
else
/* TRANSLATORS: most font transformations apply only to
@@ -589,6 +620,7 @@ add_font (struct grub_font_info *font_info, FT_Face face,
int nocut)
{
struct gsub_header *gsub = NULL;
FT_ULong gsub_len = 0;
+ grub_uint8_t *gsub_end = NULL;
if (!FT_Load_Sfnt_Table (face, TTAG_GSUB, 0, NULL, _len))
{
@@ -600,6 +632,9 @@ add_font (struct grub_font_info *font_info, FT_Face face,
int nocut)
gsub_len = 0;
}
}
+
+ gsub_end = (grub_uint8_t *) gsub + gsub_len;
+
if (gsub)
{
struct gsub_features *features
@@ -610,6 +645,11
[PATCH 1/1] mkfont: Fix tainted loop boundary issues with substitutions
With gsub substitutions the offsets should be validated against the
the number of glyphs in a font face and the memory allocated for the gsub
substitution data.
Both the number of glyphs and the last address in the allocated data are
passed in to process_cursive(), where the number of glyphs validates the end
of the range.
Enabling memory allocation validation uses two macros, one to simply check the
address against the allocated space, and the other to check that the number of
items of a given size doesn't extend outside of the allocated space.
Fixes: CID 73770
Fixes: CID 314040
Signed-off-by: Darren Kenny
---
util/grub-mkfont.c | 62 +++---
1 file changed, 53 insertions(+), 9 deletions(-)
diff --git a/util/grub-mkfont.c b/util/grub-mkfont.c
index d0e5ec564f03..0a5c232d981c 100644
--- a/util/grub-mkfont.c
+++ b/util/grub-mkfont.c
@@ -67,6 +67,11 @@
#define GRUB_FONT_RANGE_BLOCK 1024
+#define GSUB_PTR_VALID(x, end) assert((grub_uint8_t*)(x) <= end)
+
+#define GSUB_ARRAY_SIZE_VALID(a, sz, end) \
+assert((sz) >= 0 && ((sz) <= (end - (grub_uint8_t*)(a)) / sizeof(*(a
+
struct grub_glyph_info
{
struct grub_glyph_info *next;
@@ -487,16 +492,22 @@ subst (const struct gsub_substitution *sub, grub_uint32_t
glyph,
static void
process_cursive (struct gsub_feature *feature,
struct gsub_lookup_list *lookups,
-grub_uint32_t feattag)
+grub_uint32_t feattag,
+grub_uint32_t num_glyphs,
+grub_uint8_t *gsub_end)
{
int j, k;
int i;
+ int lookup_count = grub_be_to_cpu16 (feature->lookupcount);
struct glyph_replace **target = NULL;
struct gsub_substitution *sub;
- for (j = 0; j < grub_be_to_cpu16 (feature->lookupcount); j++)
+ GSUB_ARRAY_SIZE_VALID(feature->lookupindices, lookup_count, gsub_end);
+
+ for (j = 0; j < lookup_count; j++)
{
int lookup_index = grub_be_to_cpu16 (feature->lookupindices[j]);
+ int sub_count;
struct gsub_lookup *lookup;
if (lookup_index >= grub_be_to_cpu16 (lookups->count))
{
@@ -538,7 +549,12 @@ process_cursive (struct gsub_feature *feature,
target = _medijoin;
break;
}
- for (k = 0; k < grub_be_to_cpu16 (lookup->subtablecount); k++)
+
+ sub_count = grub_be_to_cpu16 (lookup->subtablecount);
+
+ GSUB_ARRAY_SIZE_VALID(lookup->subtables, sub_count, gsub_end);
+
+ for (k = 0; k < sub_count; k++)
{
sub = (struct gsub_substitution *)
((grub_uint8_t *) lookup + grub_be_to_cpu16 (lookup->subtables[k]));
@@ -558,19 +574,34 @@ process_cursive (struct gsub_feature *feature,
i = 0;
if (covertype == GSUB_COVERAGE_LIST)
{
+ int count = grub_be_to_cpu16 (cover->count);
struct gsub_coverage_list *cover = coverage;
int l;
- for (l = 0; l < grub_be_to_cpu16 (cover->count); l++)
+
+ GSUB_ARRAY_SIZE_VALID(cover->glyphs, count, gsub_end);
+
+ for (l = 0; l < count; l++)
subst (sub, grub_be_to_cpu16 (cover->glyphs[l]), target, );
}
else if (covertype == GSUB_COVERAGE_RANGE)
{
+ int count = grub_be_to_cpu16 (cover->count);
struct gsub_coverage_ranges *cover = coverage;
int l, m;
- for (l = 0; l < grub_be_to_cpu16 (cover->count); l++)
- for (m = grub_be_to_cpu16 (cover->ranges[l].start);
-m <= grub_be_to_cpu16 (cover->ranges[l].end); m++)
- subst (sub, m, target, );
+
+ GSUB_ARRAY_SIZE_VALID(cover->ranges, count, gsub_end);
+
+ for (l = 0; l < count; l++)
+ {
+ grub_uint16_t start = grub_be_to_cpu16
(cover->ranges[l].start);
+ grub_uint16_t end = grub_be_to_cpu16 (cover->ranges[l].end);
+
+ if (start > end || end > num_glyphs)
+ grub_util_error ("%s", _("invalid font range"));
+
+ for (m = start; m <= end; m++)
+ subst (sub, m, target, );
+ }
}
else
/* TRANSLATORS: most font transformations apply only to
@@ -589,6 +620,7 @@ add_font (struct grub_font_info *font_info, FT_Face face,
int nocut)
{
struct gsub_header *gsub = NULL;
FT_ULong gsub_len = 0;
+ grub_uint8_t *gsub_end = NULL;
if (!FT_Load_Sfnt_Table (face, TTAG_GSUB, 0, NULL, _len))
{
@@ -600,6 +632,9 @@ add_font (struct grub_font_info *font_info, FT_Face face,
int nocut)
gsub_len = 0;
}
}
+
+ gsub_end = (grub_uint8_t *) gsub + gsub_len;
+
if (gsub)
{
struct gsub_features *features
@@ -610,6 +645,11 @@ add_font (struct grub_font_info *font_info, FT_F
[PATCH 0/1] Fix clang build, partially
To fix the build with clang, there are 2 parts needed, the first is to get a fix back into gnulib to ensure that the regex code can build, that has been submitted upstream and can be pulled in later since there is now a preference not to have patches held in GRUB's own sources but fix upstream. But even with that fix, GRUB won't build, and that is because Clang replaces the 'abort()' call with '__builtin_trap', but the abort call is surrounded by an EXPORT_FUNC(), which is valid for GCC. The core of the issue is that the gensymlist script should be filtering out any call starting with '__builtin_', so this patch will add that filter. Thanks, Darren. Darren Kenny (1): gensymlist: fix clang build with exporting of __builtin_trap grub-core/gensymlist.sh | 1 + 1 file changed, 1 insertion(+) -- 2.31.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/1] gensymlist: fix clang build with exporting of __builtin_trap
clang expands the abort function to __builtin_trap, but that cannot be
exported.
The script that generates the symlist.c file should also exclude any
symbols that start with __builtin_.
Signed-off-by: Darren Kenny
---
grub-core/gensymlist.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/grub-core/gensymlist.sh b/grub-core/gensymlist.sh
index 5074ef6aad58..a2e5b85d0a71 100644
--- a/grub-core/gensymlist.sh
+++ b/grub-core/gensymlist.sh
@@ -58,6 +58,7 @@ EOF
(while read LINE; do echo $LINE; done) \
| grep -v '^#' \
+ | grep -v 'EXPORT_FUNC *(__builtin_[a-zA-Z0-9_]*)' \
| sed -n \
-e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC
*(\([a-zA-Z0-9_]*\)).*/ {"\1", \1, 1},/;p;}' \
-e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR
*(\([a-zA-Z0-9_]*\)).*/ {"\1", (void *) \&\1, 0},/;p;}' \
--
2.31.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: GRUB coverity fixes for CIDs 314020 and 314023
On Thursday, 2022-06-02 at 15:18:25 UTC, Jagannathan Raman wrote: > Hi, > > This series addresses a couple of untrusted loop bounds flagged by > Coverity in "grub-core/fs/zfs". Both the bugs addressed in this series > are of the same type - caused by downcast of pointer from a strict type > to a less strict type. > > Please share your thoughts on this. > These changes look good to me, thanks for looking at them Jag. Reviewed-by: Darren Kenny Thanks, Darren. > Thank you! > -- > Jag > > Jagannathan Raman (2): > fs/zfs/zfs.c: make_mdn() - avoid pointer downcasting > fs/zfs/zfs.c: zfs_mount() - avoid pointer downcasting > > grub-core/fs/zfs/zfs.c | 16 +++- > 1 file changed, 7 insertions(+), 9 deletions(-) > > -- > 2.31.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/6] Fix coverity bugs and add checks for elf values in grub-core
Hi Alec, All of these look great, so: Reviewed-by: Darren Kenny Thanks for looking at the Coverity issues, Darren. On Thursday, 2022-05-26 at 15:29:46 -04, Alec Brown wrote: > Coverity identified several untrusted loop bounds and untrusted allocation > size > bugs in grub-core/loader/i386/bsdXX.c and grub-core/loader/multiboot_elfXX.c. > Upon review of these bugs, I found that specific checks weren't being made to > various elf header values based on the elf manual page. The first four patches > in this patch series address the coverity bugs, as well as adds functions to > check for the correct elf header values. The last two patches adds fixes to > previous work done in util/grub-module-verifierXX.c that also relates to > making > checks of elf header values. > > The Coverity bugs being addressed are: > CID 314018 > CID 314030 > CID 314031 > CID 314039 > > Alec Brown (6): > grub-core/loader/i386/bsdXX.c: Avoid downcasting (char *) to (Elf_Shdr > *) > elf: Validate number of elf section header table entries > elf: Validate elf section header table index for section name string > table > elf: Validate number of elf program header table entries > util/grub-module-verifierXX.c: Add e_shoff check in get_shdr() > util/grub-module-verifierXX.c: Changed get_shnum() return type > > grub-core/kern/elf.c | 18 ++ > grub-core/kern/elfXX.c | 101 > + > grub-core/loader/i386/bsdXX.c | 142 > +- > grub-core/loader/multiboot_elfxx.c | 79 > ++- > include/grub/elf.h | 23 +++ > util/grub-module-verifierXX.c | 13 + > 6 files changed, 290 insertions(+), 86 deletions(-) ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/1] grub-mkimage: Creating aarch64 images from x86 host is broken
A recent fix that made appears to have broken the ability to create an
aarch64 boot image on a x86-based host.
This was due to an overzealous testing of the architecture when building
grub-mkimage and removing the code that build an Arm image when not
built on Arm.
Fixes: 8541f319 ("grub-mkimage: Only check aarch64 relocations when built for
aarch64")
Signed-off-by: Darren Kenny
Tested-by: Selva Ganesan
---
util/grub-mkimagexx.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index 1e29e255e8d2..a1927e786928 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -1631,7 +1631,7 @@ translate_relocation_pe (struct translate_context *ctx,
}
break;
case EM_AARCH64:
-#if defined(MKIMAGE_ELF64) && defined(__arm__)
+#if defined(MKIMAGE_ELF64)
switch (ELF_R_TYPE (info))
{
case R_AARCH64_ABS64:
@@ -1667,8 +1667,7 @@ translate_relocation_pe (struct translate_context *ctx,
(unsigned int) ELF_R_TYPE (info));
break;
}
-#endif /* defined(MKIMAGE_ELF64) && define(__arm__) */
- break;
+#endif /* defined(MKIMAGE_ELF64) */
break;
#if defined(MKIMAGE_ELF32)
case EM_ARM:
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 0/5] Fix coverity bugs and add checks for elf values in grub-core
Hi Alec, This all looks good to me, so for the series: Reviewed-by: Darren Kenny Thanks, Darren. On Wednesday, 2022-04-20 at 22:23:12 -04, Alec Brown wrote: > v3: Added check for e_shoff, made starting words lowercase in error messages, > and added comment to why return pointers are set to 0. > > Coverity identified several untrusted loop bounds and untrusted allocation > size > bugs in grub-core/loader/i386/bsdXX.c and grub-core/loader/multiboot_elfXX.c. > Upon review of these bugs, I found that specific checks weren't being made to > various elf header values based on the elf manual page. This patch series > addresses the coverity bugs, as well as adds functions to check for the > correct > elf header values. > > The Coverity bugs being addressed are: > CID 314018 > CID 314030 > CID 314031 > CID 314039 > > Alec Brown (5): > grub-core/loader/i386/bsdXX.c: Avoid downcasting (char *) to (Elf_Shdr > *) > elf: Validate number of elf section header table entries > elf: Validate elf section header table index for section name string > table > elf: Validate number of elf program header table entries > util/grub-module-verifierXX.c: Add e_shoff check in get_shdr() > > grub-core/kern/elf.c | 15 +++ > grub-core/kern/elfXX.c | 101 > + > grub-core/loader/i386/bsdXX.c | 137 > + > grub-core/loader/multiboot_elfxx.c | 76 > +++- > include/grub/elf.h | 18 ++ > util/grub-module-verifierXX.c | 3 +++ > 6 files changed, 273 insertions(+), 77 deletions(-) ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/1] jpeg: Fix possible invalid loop boundary condition
The value of next_marker is adjusted based on the a word sized value
read from data->file.
The updated next_marker value should reference a location in the file
just beyond the huffman table, and as such should not have a value
larger than the size of the file.
Fixes: CID 73657
Signed-off-by: Darren Kenny
---
grub-core/video/readers/jpeg.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/grub-core/video/readers/jpeg.c b/grub-core/video/readers/jpeg.c
index e31602f766ae..c47ffd65151e 100644
--- a/grub-core/video/readers/jpeg.c
+++ b/grub-core/video/readers/jpeg.c
@@ -199,6 +199,12 @@ grub_jpeg_decode_huff_table (struct grub_jpeg_data *data)
next_marker = data->file->offset;
next_marker += grub_jpeg_get_word (data);
+ if (next_marker > data->file->size)
+{
+ return grub_error (GRUB_ERR_BAD_FILE_TYPE,
+"jpeg: invalid huffman table");
+}
+
while (data->file->offset + sizeof (count) + 1 <= next_marker)
{
id = grub_jpeg_get_byte (data);
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: clang build fails with `../grub-core/lib/gnulib/regex.h:682:20: error: variable length array used [-Werror,-Wvla]`
Hi Paul, I'm seeing that same issue, and I sent an e-mail to the gnulib list about it yesterday. I've not gotten a response to it yet though - but it is on my radar to get fixed since it's also blocking some other work I'm doing. Thanks, Darren. On Friday, 2022-03-25 at 08:12:16 +01, Paul Menzel wrote: > Dear GRUB folks, > > > On Debian sid/unstable clang 13.0.1 fails to build GRUB due to a problem > in Gnulib: > > ``` > $ git log --oneline -1 --no-decorate > 7c316e183 term/efi/console: Do not set cursor until the first text output > $ ./configure --with-platform=coreboot CC=clang && make > […] > clang -DHAVE_CONFIG_H -I. -I.. -Wall -W -DGRUB_MACHINE_COREBOOT=1 > -DGRUB_MACHINE=I386_COREBOOT -m32 -msoft-float -Xclang -msoft-float > -Xclang -no-implicit-float -nostdinc -isystem > /usr/lib/llvm-13/lib/clang/13.0.1/include -I../include -I../include > -DGRUB_FILE=\"commands/regexp.c\" -I. -I. -I.. -I.. -I../include > -I../include -I../grub-core/lib/libgcrypt-grub/src/ > -I../grub-core/lib/posix_wrap -I../grub-core/lib/gnulib > -I../grub-core/lib/gnulib -D_FILE_OFFSET_BITS=64 -std=gnu99 -fno-common > -Os -m32 -Wall -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts > -Wcomment -Wdeprecated-declarations -Wdisabled-optimization > -Wdiv-by-zero -Wfloat-equal -Wformat-extra-args -Wformat-security > -Wformat-y2k -Wimplicit -Wimplicit-function-declaration -Wimplicit-int > -Wmain -Wmissing-braces -Wmissing-format-attribute -Wmultichar > -Wparentheses -Wreturn-type -Wsequence-point -Wshadow -Wsign-compare > -Wswitch -Wtrigraphs -Wunknown-pragmas -Wunused -Wunused-function > -Wunused-label -Wunused-parameter -Wunused-value -Wunused-variable > -Wwrite-strings -Wnested-externs -Wstrict-prototypes -g > -Wredundant-decls -Wmissing-prototypes -Wmissing-declarations -Wextra > -Wattributes -Wendif-labels -Winit-self -Wint-to-pointer-cast > -Winvalid-pch -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla > -Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros > -Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs > -Wmissing-prototypes -Wmissing-declarations -Wformat=2 -march=i386 > -falign-functions=1 -freg-struct-return -mno-mmx -mno-sse -mno-sse2 > -mno-sse3 -mno-3dnow -fno-dwarf2-cfi-asm -mno-stack-arg-probe > -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-ident -Qn > -Qunused-arguments -fno-stack-protector -Werror -ffreestanding > -fno-builtin -Wno-undef -Wno-sign-compare -Wno-unused > -Wno-unused-parameter -Wno-redundant-decls -Wno-unreachable-code > -Wno-conversion -MT commands/regexp_module-regexp.o -MD -MP -MF > commands/.deps-core/regexp_module-regexp.Tpo -c -o > commands/regexp_module-regexp.o `test -f 'commands/regexp.c' || echo > './'`commands/regexp.c > In file included from commands/regexp.c:28: > ../grub-core/lib/gnulib/regex.h:682:20: error: variable length array > used [-Werror,-Wvla] > _REGEX_NELTS (__nmatch)], >^~~~ > ../grub-core/lib/gnulib/regex.h:528:27: note: expanded from macro > '_REGEX_NELTS' > # define _REGEX_NELTS(n) n >^ > 1 error generated. > make[3]: *** [Makefile:41321: commands/regexp_module-regexp.o] Fehler 1 > ``` > > I tried to use latest Gnulib (not the one defined in `bootstrap.conf`), > but the error remains. > > How to deal with that, if I am unsure how to fix it? Report it to the > Gnulib folks? > > > Kind regards, > > Paul > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 0/7] Fix coverity uninitialized scalar variable bugs in grub-core
Hi Alec,
These changes look good. For the series:
Reviewed-by: Darren Kenny
Thanks,
Darren.
On Monday, 2022-03-21 at 02:28:55 -04, Alec Brown wrote:
> v2: Set structs with multiple uninitialized members to {0} and set single
> uninitialized members to 0.
>
> Coverity identified multiple uninitialized scalar variable bugs in multiple
> components of the grub-core. These patches address these issues.
>
> The Coverity bugs being addressed are:
> CID 375026
> CID 375028
> CID 375030
> CID 375031
> CID 375033
> CID 375035
> CID 375036
>
> Alec Brown (7):
> grub-core/loader/i386/bsd.c: Fix uninitialized scalar variable
> grub-core/loader/i386/pc/linux.c: Fix uninitialized scalar variable
> grub-core/net/arp.c: Fix uninitialized scalar variable
> grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable
> grub-core/net/net.c: Fix uninitialized scalar variable
> grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable
> grub-core/net/bootp.c: Fix uninitialized scalar variable
>
> grub-core/loader/i386/bsd.c | 2 +-
> grub-core/loader/i386/pc/linux.c | 2 +-
> grub-core/loader/i386/xnu.c | 4 ++--
> grub-core/net/arp.c | 2 ++
> grub-core/net/bootp.c| 1 +
> grub-core/net/net.c | 1 +
> 6 files changed, 8 insertions(+), 4 deletions(-)
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/2] grub-mkimage: Only check aarch64 relocations when built for aarch64
Coverity flagged the switch checks for R_AARCH64_* as being logically
dead code, since it could never happen on x86 due to the masking of the
values earlier in the code.
A check for building on __ARM_ARCH (which gcc and clang define) and for
MKIMAGE_ELF64 (which GRUB defines) has been added to avoid this dead
code being built in.
Fixes: CID 158599
Signed-off-by: Darren Kenny
---
util/grub-mkimagexx.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index 9762bc80e40d..9ff31083f746 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -1631,6 +1631,7 @@ translate_relocation_pe (struct translate_context *ctx,
}
break;
case EM_AARCH64:
+#if defined(MKIMAGE_ELF64) && defined(__ARM_ARCH)
switch (ELF_R_TYPE (info))
{
case R_AARCH64_ABS64:
@@ -1666,6 +1667,7 @@ translate_relocation_pe (struct translate_context *ctx,
(unsigned int) ELF_R_TYPE (info));
break;
}
+#endif /* defined(MKIMAGE_ELF64) && define(__ARM_ARCH) */
break;
break;
#if defined(MKIMAGE_ELF32)
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/2] kern: Ensure that parser allocated memory is not leaked
While it would appear unlikely that the memory allocated in *argv in
grub_parser_split_cmdline() would be leaked, we should try ensure that
it doesn't leak by calling grub_free() before we return from
grub_rescue_parse_line().
To avoid a possible double-free, grub_parser_split_cmdline() is being
changed to assign *argv = NULL when we've called grub_free() in the fail
section.
Fixes: CID 96680
Signed-off-by: Darren Kenny
---
grub-core/kern/parser.c| 2 ++
grub-core/kern/rescue_parser.c | 10 --
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
index 6ab7aa427cca..9b7b31a5162f 100644
--- a/grub-core/kern/parser.c
+++ b/grub-core/kern/parser.c
@@ -298,6 +298,8 @@ grub_parser_split_cmdline (const char *cmdline,
fail:
grub_free (*argv);
+ *argv = NULL;
+ *argc = 0;
goto out;
}
diff --git a/grub-core/kern/rescue_parser.c b/grub-core/kern/rescue_parser.c
index 63383669977a..3520aed40668 100644
--- a/grub-core/kern/rescue_parser.c
+++ b/grub-core/kern/rescue_parser.c
@@ -36,10 +36,16 @@ grub_rescue_parse_line (char *line,
if (grub_parser_split_cmdline (line, getline, getline_data, , )
|| n < 0)
-return grub_errno;
+{
+ grub_free(args);
+ return grub_errno;
+}
if (n == 0)
-return GRUB_ERR_NONE;
+{
+ grub_free(args);
+ return GRUB_ERR_NONE;
+}
/* In case of an assignment set the environment accordingly
instead of calling a function. */
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/7] Fix coverity uninitialized scalar variable bugs in grub-core
Hi Alec, Thanks for doing these changes, it all looks good, so for the series: Reviewed-by: Darren Kenny Thanks, Darren. On Tuesday, 2022-03-15 at 16:24:02 -04, Alec Brown wrote: > Coverity identified multiple uninitialized scalar variable bugs in multiple > components of the grub-core. These patches address these issues. > > The Coverity bugs being addressed are: > CID 375026 > CID 375028 > CID 375030 > CID 375031 > CID 375033 > CID 375035 > CID 375036 > > Alec Brown (7): > grub-core/loader/i386/bsd.c: Fix uninitialized scalar variable > grub-core/loader/i386/pc/linux.c: Fix uninitialized scalar variable > grub-core/net/arp.c: Fix uninitialized scalar variable > grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable > grub-core/net/net.c: Fix uninitialized scalar variable > grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable > grub-core/net/bootp.c: Fix uninitialized scalar variable > > grub-core/loader/i386/bsd.c | 2 +- > grub-core/loader/i386/pc/linux.c | 2 +- > grub-core/loader/i386/xnu.c | 4 ++-- > grub-core/net/arp.c | 3 ++- > grub-core/net/bootp.c| 2 +- > grub-core/net/net.c | 2 +- > 6 files changed, 8 insertions(+), 7 deletions(-) ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] affs: Fix resource leaks
Hi Alec,
Don't know how I ended up missing this originally, but thanks for fixing
it, so:
Reviewed-by: Darren Kenny
Thanks,
Darren.
On Wednesday, 2022-02-02 at 19:08:21 -05, Alec Brown wrote:
> In commit 178ac5107389 (affs: Fix memory leaks), fixes were made to
> grub_affs_iterate_dir() to prevent memory leaks from occuring after it returns
> without freeing node. However, there were still some instances where node was
> causing a memory leak when the function returns after calling
> grub_affs_create_node(). In this function, new memory is allocated to node but
> doesn't get freed until the hook() function is called near the end. Before
> hook() is called, node should be freed in grub_affs_create_node() before
> returning out of it.
>
> Fixes: 178ac5107389 (affs: Fix memory leaks)
> Fixes: CID 73759
>
> Signed-off-by: Alec Brown
> ---
> grub-core/fs/affs.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/fs/affs.c b/grub-core/fs/affs.c
> index cafcd0fba..7b9e62064 100644
> --- a/grub-core/fs/affs.c
> +++ b/grub-core/fs/affs.c
> @@ -370,17 +370,26 @@ grub_affs_create_node (grub_fshelp_node_t dir,
> GRUB_DISK_SECTOR_SIZE -
> GRUB_AFFS_FILE_LOCATION,
> sizeof ((*node)->di), (char *) &(*node)->di);
> if (err)
> - return 1;
> + {
> + grub_free (*node);
> + return 1;
> + }
> continue;
> }
> default:
> - return 0;
> + {
> + grub_free (*node);
> + return 0;
> + }
> }
>break;
> }
>
>if (nest == 8)
> -return 0;
> +{
> + grub_free (*node);
> + return 0;
> +}
>
>type |= GRUB_FSHELP_CASE_INSENSITIVE;
>
> --
> 2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/4] Clean up code and fix coverity bugs in util/grub-module-verifierXX.c
Hi Alec, These look good to me, thanks for handling the Coverity issues here. For the series: Reviewed-by: Darren Kenny Thanks, Darren. On Wednesday, 2022-02-02 at 19:26:56 -05, Alec Brown wrote: > Coverity identified several untrusted loop bounds in > util/grub-module-verifierXX.c. This patch series addresses these bugs, cleans > up > lengthy equations, and makes checks to values based on the elf manual page. > > The Coverity Bugs being addressed are: > CID 314021 > CID 314027 > CID 314033 > > Alec Brown (4): > util/grub-module-verifierXX.c: Add function to calculate section headers > util/grub-module-verifierXX.c: Validate number of elf section header > table entries > util/grub-module-verifierXX.c: Validate elf section header table index > for section name string table > util/grub-module-verifierXX.c: Add module_size parameter to functions > for sanity checking > > util/grub-module-verifierXX.c | 124 > +--- > 1 file changed, 93 insertions(+), 31 deletions(-) ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drop gnulib fix-base64.patch
Hi Robbie,
Just tried it in a build, and it works for me.
On Thursday, 2021-10-28 at 15:22:27 -04, Robbie Harwood wrote:
> Originally added in 9fbdec2f6b4fa8b549daa4d49134d1fe89d95ef9 and
> subsequently modified in 552c9fd08122a3036c724ce96dfe68aa2f75705f,
> fix-base64.patch handled two problems we have using gnulib, which are
> exerciesd by the base64 module but not directly caused by it.
>
> First, grub2 defines its own bool type, while gnulib expects the
> equivalent of stdbool.h to be present. Rather than patching gnulib,
> instead use gnulib's stdbool module to provide a bool type if needed.
> (Suggested by Simon Josefsson.)
>
> Second, our config.h doesn't always inherit config-util.h, which is
> where gnulib-related options like _GL_ATTRIBUTE_CONST end up.
> fix-base64.h worked around this by defining the attribute away, but this
> workaround is better placed in config.h itself, not a gnulib patch.
>
> Signed-off-by: Robbie Harwood
Reviewed-by: Darren Kenny
Thanks,
Darren.
> ---
> bootstrap.conf| 3 ++-
> config.h.in | 3 +++
> grub-core/lib/gnulib-patches/fix-base64.patch | 21 ---
> grub-core/lib/posix_wrap/sys/types.h | 7 +++
> grub-core/lib/xzembed/xz.h| 5 +
> 5 files changed, 9 insertions(+), 30 deletions(-)
> delete mode 100644 grub-core/lib/gnulib-patches/fix-base64.patch
>
> diff --git a/bootstrap.conf b/bootstrap.conf
> index 0dd893c5c..21a8cf15d 100644
> --- a/bootstrap.conf
> +++ b/bootstrap.conf
> @@ -35,6 +35,7 @@ gnulib_modules="
>realloc-gnu
>regex
>save-cwd
> + stdbool
> "
>
> gnulib_tool_option_extras="\
> @@ -80,7 +81,7 @@ cp -a INSTALL INSTALL.grub
>
> bootstrap_post_import_hook () {
>set -e
> - for patchname in fix-base64 fix-null-deref fix-null-state-deref
> fix-regcomp-uninit-token \
> + for patchname in fix-null-deref fix-null-state-deref
> fix-regcomp-uninit-token \
>fix-regexec-null-deref fix-uninit-structure fix-unused-value fix-width
> no-abort; do
> patch -d grub-core/lib/gnulib -p2 \
>< "grub-core/lib/gnulib-patches/$patchname.patch"
> diff --git a/config.h.in b/config.h.in
> index 9e8f9911b..2b65c86c4 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -64,4 +64,7 @@
>
> #define _GNU_SOURCE 1
>
> +/* For gnulib's base64 code. */
> +#define _GL_ATTRIBUTE_CONST /* empty */
> +
> #endif
> diff --git a/grub-core/lib/gnulib-patches/fix-base64.patch
> b/grub-core/lib/gnulib-patches/fix-base64.patch
> deleted file mode 100644
> index 985db1279..0
> --- a/grub-core/lib/gnulib-patches/fix-base64.patch
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -diff --git a/lib/base64.h b/lib/base64.h
> -index 9cd0183b8..185a2afa1 100644
> a/lib/base64.h
> -+++ b/lib/base64.h
> -@@ -21,8 +21,14 @@
> - /* Get size_t. */
> - # include
> -
> --/* Get bool. */
> --# include
> -+#ifndef GRUB_POSIX_BOOL_DEFINED
> -+typedef enum { false = 0, true = 1 } bool;
> -+#define GRUB_POSIX_BOOL_DEFINED 1
> -+#endif
> -+
> -+#ifndef _GL_ATTRIBUTE_CONST
> -+# define _GL_ATTRIBUTE_CONST /* empty */
> -+#endif
> -
> - # ifdef __cplusplus
> - extern "C" {
> diff --git a/grub-core/lib/posix_wrap/sys/types.h
> b/grub-core/lib/posix_wrap/sys/types.h
> index 854eb0122..eeda543c4 100644
> --- a/grub-core/lib/posix_wrap/sys/types.h
> +++ b/grub-core/lib/posix_wrap/sys/types.h
> @@ -23,11 +23,10 @@
>
> #include
>
> +/* Provided by gnulib if not present. */
> +#include
> +
> typedef grub_ssize_t ssize_t;
> -#ifndef GRUB_POSIX_BOOL_DEFINED
> -typedef enum { false = 0, true = 1 } bool;
> -#define GRUB_POSIX_BOOL_DEFINED 1
> -#endif
>
> typedef grub_uint8_t uint8_t;
> typedef grub_uint16_t uint16_t;
> diff --git a/grub-core/lib/xzembed/xz.h b/grub-core/lib/xzembed/xz.h
> index f7b32d800..d1417039a 100644
> --- a/grub-core/lib/xzembed/xz.h
> +++ b/grub-core/lib/xzembed/xz.h
> @@ -29,10 +29,7 @@
> #include
> #include
> #include
> -
> -#ifndef GRUB_POSIX_BOOL_DEFINED
> -typedef enum { false = 0, true = 1 } bool;
> -#endif
> +#include
>
> /**
> * enum xz_ret - Return codes
> --
> 2.33.0
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/6] Fix some Coverity low-hanging bugs
Coverity has flagged a number of small issues that should be fixed to help in cleaning up the code - these here are primarily memory leaks or uninitialized variables. In theory leaked memory is significant, but for short-lived processes it is minor. Similarly for unitinialized variables - some compilers will do the right thing and zero out the value allocated on the stack, but some won't. So it is better to be sure of the content that leave it open for possible misuse. Darren Kenny (6): grub-install-common: Fix memory leak in copy_all() grub-mkrescue: Fix memory leak in write_part() grub-fstest: Fix resource leaks in cmd_cmp() grub-mkfont: Fix memory leak in write_font_pf2() zfs: Fix possible insecure use of chunk size in zap_leaf_array_get() gzio: Fix possible use of uninitialized variable in huft_build() grub-core/fs/zfs/zfs.c | 3 ++- grub-core/io/gzio.c| 2 +- util/grub-fstest.c | 7 ++- util/grub-install-common.c | 4 +++- util/grub-mkfont.c | 1 + util/grub-mkrescue.c | 1 + 6 files changed, 14 insertions(+), 4 deletions(-) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 5/6] zfs: Fix possible insecure use of chunk size in zap_leaf_array_get()
In zap_leaf_array_get() the chunk size passed in is considered tainted
by Coverity, and is being used before it is tested for validity.
To fix this the assignment of 'la' is moved until after the test of the
value of 'chunk'.
Fixes: CID 314014
Signed-off-by: Darren Kenny
---
grub-core/fs/zfs/zfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index 44e4e18147af..e9d7a7d0e4f6 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -2229,7 +2229,7 @@ zap_leaf_array_get (zap_leaf_phys_t * l,
grub_zfs_endian_t endian, int blksft,
while (bseen < array_len)
{
- struct zap_leaf_array *la = _LEAF_CHUNK (l, blksft, chunk)->l_array;
+ struct zap_leaf_array *la;
grub_size_t toread = array_len - bseen;
if (toread > ZAP_LEAF_ARRAY_BYTES)
@@ -2239,6 +2239,7 @@ zap_leaf_array_get (zap_leaf_phys_t * l,
grub_zfs_endian_t endian, int blksft,
/* Don't use grub_error because this error is to be ignored. */
return GRUB_ERR_BAD_FS;
+ la = _LEAF_CHUNK (l, blksft, chunk)->l_array;
grub_memcpy (buf + bseen,la->la_array, toread);
chunk = grub_zfs_to_cpu16 (la->la_next, endian);
bseen += toread;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/6] grub-install-common: Fix memory leak in copy_all()
The copy_all() function skips a section of code using continue, but
fails to free the memory in srcf first, leaking it.
Fixes: CID 314026
Signed-off-by: Darren Kenny
---
util/grub-install-common.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/util/grub-install-common.c b/util/grub-install-common.c
index 4e212e690c52..0995fa741834 100644
--- a/util/grub-install-common.c
+++ b/util/grub-install-common.c
@@ -753,8 +753,10 @@ copy_all (const char *srcd,
continue;
srcf = grub_util_path_concat (2, srcd, de->d_name);
if (grub_util_is_special_file (srcf)
- || grub_util_is_directory (srcf))
+ || grub_util_is_directory (srcf)) {
+ free(srcf);
continue;
+ }
dstf = grub_util_path_concat (2, dstd, de->d_name);
grub_install_compress_file (srcf, dstf, 1);
free (srcf);
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/6] grub-mkrescue: Fix memory leak in write_part()
In the function write_part(), the value of 'inname' is not used beyond the grub_util_fopen() call, so is should be freed to avoid leakage. Fixes: CID 314028 Signed-off-by: Darren Kenny --- util/grub-mkrescue.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c index fb4dcc6d58f7..d5e4c92fd74d 100644 --- a/util/grub-mkrescue.c +++ b/util/grub-mkrescue.c @@ -229,6 +229,7 @@ write_part (FILE *f, const char *srcdir) char *inname = grub_util_path_concat (2, srcdir, "partmap.lst"); char buf[260]; in = grub_util_fopen (inname, "rb"); + free(inname); if (!in) return; while (fgets (buf, 256, in)) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 6/6] gzio: Fix possible use of uninitialized variable in huft_build()
In huft_build() it is possible to reach the for loop where 'r' is being
assigned to 'q[j]' without 'r.v' ever being initialized.
Fixes: CID 314024
Signed-off-by: Darren Kenny
---
grub-core/io/gzio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/grub-core/io/gzio.c b/grub-core/io/gzio.c
index aea86a0a9a92..10156e569c85 100644
--- a/grub-core/io/gzio.c
+++ b/grub-core/io/gzio.c
@@ -447,7 +447,7 @@ huft_build (unsigned *b,/* code lengths in bits (all
assumed <= BMAX) */
int l; /* bits per table (returned in m) */
register unsigned *p;/* pointer into c[], b[], or v[] */
register struct huft *q; /* points to current table */
- struct huft r; /* table entry for structure assignment */
+ struct huft r = {0}; /* table entry for structure assignment */
struct huft *u[BMAX];/* table stack */
unsigned v[N_MAX]; /* values in order of bit length */
register int w; /* bits before this table == (l * h) */
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 4/6] grub-mkfont: Fix memory leak in write_font_pf2()
In the function write_font_pf2() memory is allocated for 'font_name' to construct a new name, but it is not released before returning from the function, leaking the allocated memory. Fixes: CID 314015 Signed-off-by: Darren Kenny --- util/grub-mkfont.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/grub-mkfont.c b/util/grub-mkfont.c index 0fe45a6103dd..97e8d27e91d6 100644 --- a/util/grub-mkfont.c +++ b/util/grub-mkfont.c @@ -928,6 +928,7 @@ write_font_pf2 (struct grub_font_info *font_info, char *output_file) file, output_file); } + free(font_name); fclose (file); } -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/6] grub-fstest: Fix resource leaks in cmd_cmp()
In the function cmd_cmp() within the while loop, srcnew and destnew are
being allocated but are never freed either before leaving scope or in
the recursive calls being made to cmd_cmp().
Fixes: CID 314032
Fixes: CID 314045
Signed-off-by: Darren Kenny
---
util/grub-fstest.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/util/grub-fstest.c b/util/grub-fstest.c
index 838656420098..da0751222f88 100644
--- a/util/grub-fstest.c
+++ b/util/grub-fstest.c
@@ -299,10 +299,15 @@ cmd_cmp (char *src, char *dest)
*ptr++ = '/';
strcpy (ptr, entry->d_name);
- if (grub_util_is_special_file (destnew))
+ if (grub_util_is_special_file (destnew)) {
+ free(srcnew);
+ free(destnew);
continue;
+ }
cmd_cmp (srcnew, destnew);
+ free(srcnew);
+ free(destnew);
}
grub_util_fd_closedir (dir);
return;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [SECURITY PATCH 029/117] zstd: Initialize seq_t structure fully
Hi Paul,
On Thursday, 2021-03-18 at 09:50:00 +01, Paul Menzel wrote:
> Dear Darren, dear Daniel,
>
>
> Am 02.03.21 um 19:00 schrieb Daniel Kiper:
>> From: Darren Kenny
>>
>> While many compilers will initialize this to zero, not all will,
>
> Which ones do not?
I have been working with C for a long time now, and there have been
compilers that don't initialize stack variables.
While many of the Linux compilers such as gcc and clang compilers often
do - depending on the optimization levels and building with debug or
not. (Debug code tends to be correctly zeroed out, but optimized doesn't
always)
I remember having to change a lot of GNOME C code being ported to
another platform, Solaris at the time, where that code always assumed
that it would be initialized to 0, and random things would happen
depending on what was on the stack.
The details of this are well documented and are a known security issue:
- CWE-457 (http://cwe.mitre.org/data/definitions/457.html)
>> so it is better to be sure that fields not being explicitly set are at known
>> values, and there is code that checks this fields value elsewhere in the
>> code.
>>
>> Fixes: CID 292440
>
> What is the exact error? Is there a code flow, where one element does
> not get set. (The commit message would be incorrect if this is not the
> case.)
I can't honestly remember the details that Coverity had, but there was a
flow found by Coverity that did end up returning the value in seq
without touching all of the fields in the structure.
Looking through the code manually just now, I can see that seq.match is
never set during that code in ZSTD_decodeSequence(), so that is the most
likely cause of the error from Coverity.
All that it can do is report that it was not set before returning, it
cannot necessarily predict what will happen after that function returns
it, especially over time.
>
> Lastly, this is imported from upstream. I created an issue upstream [1].
>
Thanks, makes sense.
>> Signed-off-by: Darren Kenny
>> Reviewed-by: Daniel Kiper
>> ---
>> grub-core/lib/zstd/zstd_decompress.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/grub-core/lib/zstd/zstd_decompress.c
>> b/grub-core/lib/zstd/zstd_decompress.c
>> index 711b5b6d7..e4b5670c2 100644
>> --- a/grub-core/lib/zstd/zstd_decompress.c
>> +++ b/grub-core/lib/zstd/zstd_decompress.c
>> @@ -1325,7 +1325,7 @@ typedef enum { ZSTD_lo_isRegularOffset,
>> ZSTD_lo_isLongOffset=1 } ZSTD_longOffset
>> FORCE_INLINE_TEMPLATE seq_t
>> ZSTD_decodeSequence(seqState_t* seqState, const ZSTD_longOffset_e
>> longOffsets)
>> {
>> -seq_t seq;
>> +seq_t seq = {0};
>> U32 const llBits =
>> seqState->stateLL.table[seqState->stateLL.state].nbAdditionalBits;
>> U32 const mlBits =
>> seqState->stateML.table[seqState->stateML.state].nbAdditionalBits;
>> U32 const ofBits =
>> seqState->stateOffb.table[seqState->stateOffb.state].nbAdditionalBits;
>>
>
> I once read, that compilers cannot warn you, if you miss setting an
> element if you initialize structures to 0 in the beginning.
>
That is true, since it has been initialized :)
But compilers won't always warn you either unless you ask them to, e.g.
gcc requires you to add the -Wuninitialized option, it isn't included in
-Wall (at lot aren't TBH, despite the name).
It is fine not to initialize while developing the code - if you want to
use that to catch such cases - but you should probably then also be
enabling every possible warning that a compiler may provide, maybe also
using some static code analysis (though compilers are improving a lot
here, e.g. in GCC 10) and most importantly fix them all before shipping
code :)
All to common is that people end up turning off the annoying warnings
rather than paying attention to them and resolving them.
Thanks,
Darren.
> Kind regards,
>
> Paul
>
>
> [1]: https://github.com/facebook/zstd/issues/2545
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
