Re: [PATCH 1/1] pxe: simplify label_boot()

[Tom Rini]

On Tue, Nov 16, 2021 at 03:09:58PM +0800, Art Nikpal wrote:
> On Tue, Nov 16, 2021 at 2:26 AM Heinrich Schuchardt
>  wrote:
> >
> > Coverity CID 131256 indicates a possible buffer overflow in label_boot().
> > This would only occur if the size of the downloaded file would exceed 4
> > GiB. But anyway we can simplify the code by using snprintf() and checking
> > the return value.
> >
> > Signed-off-by: Heinrich Schuchardt 
> > ---
> >  boot/pxe_utils.c | 9 -
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> > index a7a84f26c1..5841561bdf 100644
> > --- a/boot/pxe_utils.c
> > +++ b/boot/pxe_utils.c
> > @@ -465,11 +465,10 @@ static int label_boot(struct pxe_context *ctx, struct 
> > pxe_label *label)
> > }
> >
> > initrd_addr_str = env_get("ramdisk_addr_r");
> > -   strcpy(initrd_filesize, simple_xtoa(size));
> > -
> > -   strncpy(initrd_str, initrd_addr_str, 18);
> > -   strcat(initrd_str, ":");
> > -   strncat(initrd_str, initrd_filesize, 9);
> > +   size = snprintf(initrd_str, sizeof(initrd_str), "%s:%lx",
> > +   initrd_addr_str, size);
> > +   if (size >= sizeof(initrd_str))
> > +   return 1;
> > }
> >
> > if (get_relfile_envaddr(ctx, label->kernel, "kernel_addr_r",
> 
> Reviewed-by: Artem Lapkin 

Thanks for the review everyone and thanks for the patch Heinrich.  It
looks like in the kernel the new'ish standard tag is:
Addresses-Coverity-ID: 131256 ("Security best practices violations")
and then not need to mention Coverity in the main message.  I'll fix
things up slightly when applying.

-- 
Tom


signature.asc
Description: PGP signature

Re: [PATCH 1/1] pxe: simplify label_boot()

[Art Nikpal]

On Tue, Nov 16, 2021 at 2:26 AM Heinrich Schuchardt
 wrote:
>
> Coverity CID 131256 indicates a possible buffer overflow in label_boot().
> This would only occur if the size of the downloaded file would exceed 4
> GiB. But anyway we can simplify the code by using snprintf() and checking
> the return value.
>
> Signed-off-by: Heinrich Schuchardt 
> ---
>  boot/pxe_utils.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index a7a84f26c1..5841561bdf 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -465,11 +465,10 @@ static int label_boot(struct pxe_context *ctx, struct 
> pxe_label *label)
> }
>
> initrd_addr_str = env_get("ramdisk_addr_r");
> -   strcpy(initrd_filesize, simple_xtoa(size));
> -
> -   strncpy(initrd_str, initrd_addr_str, 18);
> -   strcat(initrd_str, ":");
> -   strncat(initrd_str, initrd_filesize, 9);
> +   size = snprintf(initrd_str, sizeof(initrd_str), "%s:%lx",
> +   initrd_addr_str, size);
> +   if (size >= sizeof(initrd_str))
> +   return 1;
> }
>
> if (get_relfile_envaddr(ctx, label->kernel, "kernel_addr_r",
> --
> 2.32.0
>

Reviewed-by: Artem Lapkin 

Re: [PATCH 1/1] pxe: simplify label_boot()

[Ramon Fried]

On Mon, Nov 15, 2021 at 8:26 PM Heinrich Schuchardt
 wrote:
>
> Coverity CID 131256 indicates a possible buffer overflow in label_boot().
> This would only occur if the size of the downloaded file would exceed 4
> GiB. But anyway we can simplify the code by using snprintf() and checking
> the return value.
>
> Signed-off-by: Heinrich Schuchardt 
> ---
>  boot/pxe_utils.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index a7a84f26c1..5841561bdf 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -465,11 +465,10 @@ static int label_boot(struct pxe_context *ctx, struct 
> pxe_label *label)
> }
>
> initrd_addr_str = env_get("ramdisk_addr_r");
> -   strcpy(initrd_filesize, simple_xtoa(size));
> -
> -   strncpy(initrd_str, initrd_addr_str, 18);
> -   strcat(initrd_str, ":");
> -   strncat(initrd_str, initrd_filesize, 9);
> +   size = snprintf(initrd_str, sizeof(initrd_str), "%s:%lx",
> +   initrd_addr_str, size);
> +   if (size >= sizeof(initrd_str))
> +   return 1;
> }
>
> if (get_relfile_envaddr(ctx, label->kernel, "kernel_addr_r",
> --
> 2.32.0
>
Reviewed-by: Ramon Fried 

[PATCH 1/1] pxe: simplify label_boot()

[Heinrich Schuchardt]

Coverity CID 131256 indicates a possible buffer overflow in label_boot().
This would only occur if the size of the downloaded file would exceed 4
GiB. But anyway we can simplify the code by using snprintf() and checking
the return value.

Signed-off-by: Heinrich Schuchardt 
---
 boot/pxe_utils.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index a7a84f26c1..5841561bdf 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -465,11 +465,10 @@ static int label_boot(struct pxe_context *ctx, struct 
pxe_label *label)
}
 
initrd_addr_str = env_get("ramdisk_addr_r");
-   strcpy(initrd_filesize, simple_xtoa(size));
-
-   strncpy(initrd_str, initrd_addr_str, 18);
-   strcat(initrd_str, ":");
-   strncat(initrd_str, initrd_filesize, 9);
+   size = snprintf(initrd_str, sizeof(initrd_str), "%s:%lx",
+   initrd_addr_str, size);
+   if (size >= sizeof(initrd_str))
+   return 1;
}
 
if (get_relfile_envaddr(ctx, label->kernel, "kernel_addr_r",
-- 
2.32.0