Re: [PATCH v3 1/3] Add helper functions to interact with android bootimg disks.

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 08.02.2016 21:47, Shea Levy wrote:
> Currently, the kernel, command line, and ramdisk can be extracted.
> ---
>  grub-core/Makefile.core.def        |   1 +
>  grub-core/loader/android_bootimg.c | 253 
> +++++++++++++++++++++++++++++++++++++
>  include/grub/android_bootimg.h     |  12 ++
>  3 files changed, 266 insertions(+)
>  create mode 100644 grub-core/loader/android_bootimg.c
>  create mode 100644 include/grub/android_bootimg.h
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 0cc40bb..991adad 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1669,6 +1669,7 @@ module = {
>    arm64 = loader/arm64/linux.c;
>    common = loader/linux.c;
>    common = lib/cmdline.c;
> +  common = loader/android_bootimg.c;
>    enable = noemu;
>  };
>  
> diff --git a/grub-core/loader/android_bootimg.c 
> b/grub-core/loader/android_bootimg.c
> new file mode 100644
> index 0000000..bbee915
> --- /dev/null
> +++ b/grub-core/loader/android_bootimg.c
> @@ -0,0 +1,253 @@
> +/* android_bootimg.c - Helpers for interacting with the android bootimg 
> format. */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2016 Free Software Foundation, Inc.
> + *
> + *  This program 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.
> + *
> + *  This program 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 this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/file.h>
> +#include <grub/misc.h>
> +#include <grub/android_bootimg.h>
> +
> +/* from 
> https://android.googlesource.com/platform/system/core/+/506d233e7ac8ca4efa80768153d842c296477f99/mkbootimg/bootimg.h
>  */
Parts of the file cannot be effectively under different licenses. Please
put this into separate header file.
> +/* From here until the end of struct boot_img_hdr available under the 
> following terms:
> + *
> + * Copyright 2007, The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +#define BOOT_MAGIC   "ANDROID!"
> +#define BOOT_MAGIC_SIZE      8
> +#define BOOT_NAME_SIZE       16
> +#define BOOT_EXTRA_ARGS_SIZE 1024
> +
> +struct boot_img_hdr
> +{
> +  grub_uint8_t magic[BOOT_MAGIC_SIZE];
> +
> +  grub_uint32_t kernel_size;
> +  grub_uint32_t kernel_addr;
> +
> +  grub_uint32_t ramdisk_size;
> +  grub_uint32_t ramdisk_addr;
> +
> +  grub_uint32_t second_size;
> +  grub_uint32_t second_addr;
> +
> +  grub_uint32_t tags_addr;
> +  grub_uint32_t page_size;
> +  grub_uint32_t unused[2];
> +
> +  grub_uint8_t name[BOOT_NAME_SIZE];
> +
> +  grub_uint8_t cmdline[BOOT_ARGS_SIZE];
> +
> +  grub_uint32_t id[8];
> +
> +  grub_uint8_t extra_cmdline[BOOT_EXTRA_ARGS_SIZE];
> +} GRUB_PACKED;
> +
> +static grub_err_t
> +read_hdr (grub_disk_t disk, struct boot_img_hdr *hd)
> +{
> +  if (grub_disk_read (disk, 0, 0, sizeof *hd, hd))
> +    goto fail;
> +
> +  if (grub_memcmp (hd->magic, BOOT_MAGIC, BOOT_MAGIC_SIZE))
> +    goto fail;
> +
> +  if (hd->unused[0] || hd->unused[1])
> +    goto fail;
> +
> +  hd->kernel_size = grub_le_to_cpu32 (hd->kernel_size);
> +  hd->kernel_addr = grub_le_to_cpu32 (hd->kernel_addr);
> +  hd->ramdisk_size = grub_le_to_cpu32 (hd->ramdisk_size);
> +  hd->ramdisk_addr = grub_le_to_cpu32 (hd->ramdisk_addr);
> +  hd->second_size = grub_le_to_cpu32 (hd->second_size);
> +  hd->second_addr = grub_le_to_cpu32 (hd->second_addr);
> +  hd->tags_addr = grub_le_to_cpu32 (hd->tags_addr);
> +  hd->page_size = grub_le_to_cpu32 (hd->page_size);
> +
> +  grub_size_t i;
> +  for (i = 0; i < sizeof hd->id / sizeof hd->id[0]; ++i)
> +    {
> +      hd->id[i] = grub_le_to_cpu32 (hd->id[i]);
> +    }
Why do you need this byteswap? Why not byteswap when it's used?
Also in loaders you can avoid byteswap usually as you know that you load
the file for the same arch as you currently run. Exceptions are arm-be,
ppc-le and fat binary headers.
> +
> +  return GRUB_ERR_NONE;
> +
> +fail:
> +  return grub_error (GRUB_ERR_BAD_FS, N_("%s not an android bootimg"),
> +                     disk->name);
BAD_FS is not correct anymore. BAD_KERNEL would be more correct.
> +}
> +
> +static grub_ssize_t
> +grub_android_bootimg_read (grub_file_t file, char *buf, grub_size_t len)
> +{
> +  grub_size_t len_left = file->size - file->offset;
> +  len = len > len_left ? len_left : len;
> +  grub_off_t *begin_offset = file->data;
> +  grub_off_t actual_offset = *begin_offset + file->offset;
> +  file->device->disk->read_hook = file->read_hook;
> +  file->device->disk->read_hook_data = file->read_hook_data;
> +  grub_errno = grub_disk_read (file->device->disk, 0, actual_offset, len, 
> buf);
> +  file->device->disk->read_hook = 0;
> +
> +  if (grub_errno)
> +    return -1;
> +  else
> +    return len;
> +}
> +
> +static grub_err_t
> +grub_android_bootimg_close (grub_file_t file)
> +{
> +  grub_free (file->data);
> +  return GRUB_ERR_NONE;
> +}
> +
> +static struct grub_fs grub_android_bootimg_fs =
> +  {
> +    .name = "android_bootimg",
> +    .read = grub_android_bootimg_read,
> +    .close = grub_android_bootimg_close
> +  };
> +
please use grub_file_offset_open rather than reimplementing it if you
need it at all. I've looked in linux.c and you'll need only to adjust
prot_file_size calculation and grub_file_seek (add a call and adjust one
call). You can easily extract grub_cmd_linux essence into load_linux
(grub_file_t file, grub_off_t off, grub_off_t size)

> +grub_err_t
> +grub_android_bootimg_load_kernel (const char *disk_path, grub_file_t *file,
> +                                  char *cmdline)
> +{
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  struct grub_file *f = 0;
> +  grub_disk_t disk = grub_disk_open (disk_path);
> +  if (!disk)
> +    goto err;
> +
> +  struct boot_img_hdr hd;
> +  if (read_hdr (disk, &hd))
> +    goto err;
> +
> +  f = grub_zalloc (sizeof *f);
> +  if (!f)
> +    goto err;
> +
> +  f->fs = &grub_android_bootimg_fs;
> +  f->device = grub_zalloc (sizeof *(f->device));
> +  if (!f->device)
> +    goto err;
> +  f->device->disk = disk;
> +
> +  f->name = grub_malloc (sizeof "kernel");
> +  if (!f->name)
> +    goto err;
> +  grub_memcpy (f->name, "kernel", sizeof "kernel");
> +  grub_off_t *begin_offset = grub_malloc (sizeof *begin_offset);
> +  if (!begin_offset)
> +    goto err;
> +  *begin_offset = hd.page_size;
> +  f->data = begin_offset;
> +  f->size = hd.kernel_size;
> +
> +  *file = f;
All of this will go away, so I won't comment on it.
> +grub_err_t
> +grub_android_bootimg_load_initrd (const char *disk_path, grub_file_t *file)
> +{

Instead you can extract grub_cmd_initrd essence into sth like
void *prepare_initrd (grub_size_t size)
which returns a pointer to buffer where the initrd would do.
> diff --git a/include/grub/android_bootimg.h b/include/grub/android_bootimg.h
> new file mode 100644
> index 0000000..ff14df7
> --- /dev/null
> +++ b/include/grub/android_bootimg.h
> @@ -0,0 +1,12 @@
> +#include <grub/file.h>
> +
> +#define BOOT_ARGS_SIZE 512
> +
> +/* Load a kernel from a bootimg. cmdline must be at least BOOT_ARGS_SIZE */
> +grub_err_t
> +grub_android_bootimg_load_kernel (const char *disk_path, grub_file_t *file,
> +                                  char *cmdline);
> +
> +/* Load an initrd from a bootimg. */
> +grub_err_t
> +grub_android_bootimg_load_initrd (const char *disk_path, grub_file_t *file);
>

signature.asc
Description: OpenPGP digital signature