From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1UMhR5-0002uA-7p for mharc-grub-devel@gnu.org; Mon, 01 Apr 2013 12:18:19 -0400 Received: from eggs.gnu.org ([208.118.235.92]:35619) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UMhR1-0002sP-0T for grub-devel@gnu.org; Mon, 01 Apr 2013 12:18:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UMhQx-0000M1-F4 for grub-devel@gnu.org; Mon, 01 Apr 2013 12:18:14 -0400 Received: from mail-ea0-x236.google.com ([2a00:1450:4013:c01::236]:32909) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UMhQx-0000Lt-6H for grub-devel@gnu.org; Mon, 01 Apr 2013 12:18:11 -0400 Received: by mail-ea0-f182.google.com with SMTP id q15so1115189ead.41 for ; Mon, 01 Apr 2013 09:18:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=oWwM599Ti2ATHa1MsROdje3YWb4IvZQpiG4d7qFWYyc=; b=BxUFifDFCnW+ga/gMaeDTQDszMWNCRTtQ9nEss2uR2lqzbMBlNeBfzSUNRAcGJ048Q BsvXnNt/bn1Meq+tn7X4a0zEKjJWUPEg5oK2Wpl2GEwazIBWsq7dZNHa1nArsj6/usJK TRSI3mc1XfBDghsyvjqOYLFp+jr3WtCRKeihCpC2cJuxkx9efXLVluOn5iYchFLG9DS2 65GyyJGEi/ubKQOltJaOj08crQPWrdbL3KGb3Fh/hK7tYaXU5vmRnv821Sdh/qAsGfG2 VPqbWRtOmAtS0Y/QaPgz3wfmOgScd+de4tMoOaP0sRUXYowpd49zn5+ZSoUTdJCxVGwJ AwyQ== X-Received: by 10.14.179.5 with SMTP id g5mr39209038eem.41.1364833090188; Mon, 01 Apr 2013 09:18:10 -0700 (PDT) Received: from [192.168.56.2] ([151.36.245.148]) by mx.google.com with ESMTPS id t4sm22016204eel.0.2013.04.01.09.18.07 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 01 Apr 2013 09:18:09 -0700 (PDT) Message-ID: <5159B349.1020207@gmail.com> Date: Mon, 01 Apr 2013 18:18:17 +0200 From: Francesco Lavra User-Agent: Mozilla/5.0 (X11; Linux i686; rv:7.0.1) Gecko/20110929 Thunderbird/7.0.1 MIME-Version: 1.0 To: grub-devel@gnu.org Subject: Re: [PATCH 6/7] add ARM Linux loader References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4013:c01::236 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Apr 2013 16:18:18 -0000 On 03/24/2013 06:01 PM, Leif Lindholm wrote: > === added directory 'grub-core/loader/arm' > === added file 'grub-core/loader/arm/linux.c' > --- grub-core/loader/arm/linux.c 1970-01-01 00:00:00 +0000 > +++ grub-core/loader/arm/linux.c 2013-03-24 13:49:04 +0000 > @@ -0,0 +1,405 @@ > +/* linux.c - boot Linux */ > +/* > + * GRUB -- GRand Unified Bootloader > + * Copyright (C) 2013 Free Software Foundation, Inc. > + * > + * GRUB is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 3 of the License, or > + * (at your option) any later version. > + * > + * GRUB is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with GRUB. If not, see . > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +GRUB_MOD_LICENSE ("GPLv3+"); > + > +static grub_dl_t my_mod; > + > +static grub_addr_t initrd_start; > +static grub_size_t initrd_end; initrd_end should be the same type as initrd_start. > + > +static grub_addr_t linux_addr; > +static grub_size_t linux_size; > + > +static char *linux_args; > + > +static grub_addr_t firmware_boot_data; > +static grub_addr_t boot_data; > +static grub_uint32_t machine_type; > + > +/* > + * linux_prepare_fdt(): > + * Prepares a loaded FDT for being passed to Linux. > + * Merges in command line parameters and sets up initrd addresses. > + */ > +static grub_err_t > +linux_prepare_fdt (void) > +{ > + int node; > + int retval; > + int tmp_size; > + void *tmp_fdt; > + > + tmp_size = fdt_totalsize ((void *) boot_data) + FDT_ADDITIONAL_ENTRIES_SIZE; Having a fixed size limit (FDT_ADDITIONAL_ENTRIES_SIZE) to insert variable-sized data (such as the linux command line) seems suboptimal, as this may fail when a very long command line is passed. How about removing the FDT_ADDITIONAL_ENTRIES_SIZE define from the header file include/grub/arm/linux.h (after all, this is an implementation detail and IMHO shouldn't go in a public header file) and defining it in this function, say: #define FDT_ADDITIONAL_ENTRIES_SIZE (0x100 + \ grub_strlen (linux_args)) [...] > +/* > + * Only support zImage, so no relocations necessary > + */ > +static grub_err_t > +linux_load (const char *filename) > +{ > + grub_file_t file; > + int size; > + > + file = grub_file_open (filename); > + if (!file) > + return GRUB_ERR_FILE_NOT_FOUND; > + > + size = grub_file_size (file); > + if (size == 0) > + return GRUB_ERR_FILE_READ_ERROR; > + > +#ifdef GRUB_MACHINE_EFI > + linux_addr = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_PHYS_OFFSET, size); There should be a check against allocation failure. > +#else > + linux_addr = LINUX_ADDRESS; > +#endif > + grub_dprintf ("loader", "Loading Linux to 0x%08x\n", > + (grub_addr_t) linux_addr); > + > + if (grub_file_read (file, (void *) linux_addr, size) != size) > + { > + grub_printf ("Kernel read failed!\n"); > + return GRUB_ERR_FILE_READ_ERROR; In the EFI case, the allocated memory should be freed before returning. > + } > + > + if (*(grub_uint32_t *) (linux_addr + LINUX_ZIMAGE_OFFSET) > + != LINUX_ZIMAGE_MAGIC) > + { > + return grub_error (GRUB_ERR_BAD_FILE_TYPE, N_("invalid zImage")); As above, in the EFI case the allocated memory should be freed before returning. > + } > + > + linux_size = size; > + > + return GRUB_ERR_NONE; > +} > +static grub_err_t > +linux_unload (void) > +{ > + grub_dl_unref (my_mod); > + > + grub_free (linux_args); > + linux_args = NULL; linux_args might already be NULL, if memory allocation for it failed in grub_cmd_linux(). In the EFI case, the memory allocated for the kernel image should be freed as well. > + > + initrd_start = initrd_end = 0; Why should the initrd data be discarded here? > + > + return GRUB_ERR_NONE; > +} > +static grub_err_t > +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), > + int argc, char *argv[]) > +{ > + int size, retval; The return type of linux_load() is grub_err_t, so retval should be the same type. > + grub_file_t file; > + grub_dl_ref (my_mod); > + > + if (argc == 0) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected")); > + > + file = grub_file_open (argv[0]); > + if (!file) > + goto fail; > + > + retval = linux_load (argv[0]); Here the file is already open: why not just pass the file handle to linux_load()? > + grub_file_close (file); > + if (retval != GRUB_ERR_NONE) > + goto fail; In order to correctly report all error types, grub_errno should be set to retval before entering the error path. > + > + grub_loader_set (linux_boot, linux_unload, 1); Since linux_boot() may return control to its caller, the third argument of grub_loader_set() should be 0, otherwise linux_boot() returning control to its caller results in a dysfunctional state. > + > + size = grub_loader_cmdline_size (argc, argv); > + linux_args = grub_malloc (size + sizeof (LINUX_IMAGE)); > + if (!linux_args) > + goto fail; grub_loader_unset() should be called in this error path, otherwise if one tries to boot in this state, linux_prepare_fdt() will access a NULL pointer. > + > + /* Create kernel command line. */ > + grub_memcpy (linux_args, LINUX_IMAGE, sizeof (LINUX_IMAGE)); > + grub_create_loader_cmdline (argc, argv, > + linux_args + sizeof (LINUX_IMAGE) - 1, size); > + > + return GRUB_ERR_NONE; > + > +fail: > + grub_dl_unref (my_mod); > + return grub_errno; > +} > + > +static grub_err_t > +grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), > + int argc, char *argv[]) > +{ > + grub_file_t file; > + int size; > + > + if (argc == 0) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected")); > + > + file = grub_file_open (argv[0]); > + if (!file) > + return grub_errno; > + > + size = grub_file_size (file); > + if (size == 0) > + goto fail; > + > +#ifdef GRUB_MACHINE_EFI > + initrd_start = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_INITRD_PHYS_OFFSET, size); If the initrd memory was already allocated as a result of the initrd command being already called previously, it should be freed before re-allocating memory. Also there should be a check against memory allocation failure. > +#else > + initrd_start = LINUX_INITRD_ADDRESS; > +#endif > + grub_dprintf ("loader", "Loading initrd to 0x%08x\n", > + (grub_addr_t) initrd_start); > + > + if (grub_file_read (file, (void *) initrd_start, size) != size) > + goto fail; In the EFI case, the memory allocated for the initrd should be freed here. And in both EFI and U-Boot cases, initrd_start should be zeroed. > + > + initrd_end = initrd_start + size; > + > + return GRUB_ERR_NONE; > + > +fail: > + grub_file_close (file); > + > + return grub_errno; > +} > + > +static void * > +load_dtb (grub_file_t dtb, int size) > +{ > + void *fdt; > + > + fdt = grub_malloc (size); > + if (!fdt) > + return NULL; > + > + if (grub_file_read (dtb, fdt, size) != size) > + { > + grub_free (fdt); > + return NULL; > + } > + > + if (fdt_check_header (fdt) != 0) > + { > + grub_free (fdt); > + return NULL; > + } > + > + return fdt; > +} > + > +static grub_err_t > +grub_cmd_devicetree (grub_command_t cmd __attribute__ ((unused)), > + int argc, char *argv[]) > +{ > + grub_file_t dtb; > + void *blob; > + int size; > + > + if (argc != 1) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected")); > + > + dtb = grub_file_open (argv[0]); > + if (!dtb) > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("failed to open file")); > + > + size = grub_file_size (dtb); > + if (size == 0) > + goto out; > + > + blob = load_dtb (dtb, size); > + if (!blob) > + return GRUB_ERR_FILE_NOT_FOUND; > + > +#ifdef GRUB_MACHINE_EFI > + boot_data = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_FDT_PHYS_OFFSET, size); As above, if the fdt memory was already allocated as a result of the devicetree command being already called previously, it should be freed before re-allocating memory. Also there should be a check against memory allocation failure. > +#else > + boot_data = LINUX_FDT_ADDRESS; > +#endif > + grub_dprintf ("loader", "Loading device tree to 0x%08x\n", > + (grub_addr_t) boot_data); > + fdt_move (blob, (void *) boot_data, fdt_totalsize (blob)); > + grub_free (blob); I don't get the point of allocating memory for the FDT in load_dtb() just to move the FDT data to boot_data and then freeing the temporary memory. Isn't it easier if load_dtb() operates directly on boot_data? > + > + /* > + * We've successfully loaded an FDT, so any machine type passed > + * from firmware is now obsolete. > + */ > + machine_type = ARM_FDT_MACHINE_TYPE; > + > + return GRUB_ERR_NONE; The file should be closed before returning. > + > + out: > + grub_file_close (dtb); > + > + return grub_errno; > +} -- Francesco