[PATCH 1/3] Fix signedness (possible underflow) of "len" parameter for aw_fel_write_uboot_image()

[Bernhard Nortmann]

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---
fel.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fel.c b/fel.c
index 41b19c9..bbaeec9 100644
--- a/fel.c
+++ b/fel.c
@@ -215,8 +215,7 @@ void aw_fel_write(libusb_device_handle *usb, void *buf, uint32_t offset, size_t
if (uboot_size > 0 && offset <= uboot_entry + uboot_size && offset + len >= uboot_entry) {
fprintf(stderr, "ERROR: Attempt to overwrite U-Boot! "
"Request 0x%08X-0x%08X overlaps 0x%08X-0x%08X.\n",
- offset, offset + (int)len,
- uboot_entry, uboot_entry + uboot_size);
+ offset, offset + len, uboot_entry, uboot_entry + uboot_size);
exit(1);
}
aw_send_fel_request(usb, AW_FEL_1_WRITE, offset, len);
@@ -752,9 +751,11 @@ void aw_fel_write_and_execute_spl(libusb_device_handle *usb,
* image. Upon success, the image data gets transferred to the default memory
* address stored within the image header; and the function preserves the
* U-Boot entry point (offset) and size values.
+ * Note that for small files (SPL only, less than SPL_LEN_LIMIT bytes),
+ * len < 0 may be passed to this function; meaning there is no U-Boot image.
*/
void aw_fel_write_uboot_image(libusb_device_handle *usb,
- uint8_t *buf, size_t len)
+ uint8_t *buf, int len)
{
if (len <= HEADER_SIZE)
return; /* Insufficient size (no actual data), just bail out */
@@ -774,17 +775,16 @@ void aw_fel_write_uboot_image(libusb_device_handle *usb,
}
uint32_t data_size = be32toh(buf32[3]); /* Image Data Size */
uint32_t load_addr = be32toh(buf32[4]); /* Data Load Address */
- if ((size_t)data_size != len - HEADER_SIZE) {
+ if ((int)data_size != len - HEADER_SIZE) {
fprintf(stderr, "U-Boot image data size mismatch: "
- "expected %d, got %u\n", (int)len - HEADER_SIZE,
- data_size);
+ "expected %d, got %u\n", len - HEADER_SIZE, data_size);
exit(1);
}
/* TODO: Verify image data integrity using the checksum field ih_dcrc,
* available from be32toh(buf32[6])
*
* However, this requires CRC routines that mimic their U-Boot
- * counterparts, namely image_check_dcrc() in ${U-BOOT}/common/image.cabs
+ * counterparts, namely image_check_dcrc() in ${U-BOOT}/common/image.c
* and crc_wd() in ${U-BOOT}/lib/crc32.c
*
* It should be investigated if existing CRC routines in sunxi-tools
--
2.0.5

[Bernhard Nortmann]

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

fel.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/fel.c b/fel.c
index bbaeec9..95c3ee2 100644
--- a/fel.c
+++ b/fel.c
@@ -186,14 +186,20 @@ void aw_fel_print_version(libusb_device_handle *usb)
struct aw_fel_version buf;
aw_fel_get_version(usb, &buf);

+ /* Decoding the actual "SoC name" may be more complicated than this.
+ * See e.g. https://github.com/allwinner-zh/linux-3.4-sunxi/blob/master/arch/arm/mach-sunxi/sunxi-chip.c
+ */
const char *soc_name="unknown";
switch (buf.soc_id) {
- case 0x1623: soc_name="A10";break;
- case 0x1625: soc_name="A13";break;
- case 0x1633: soc_name="A31";break;
- case 0x1651: soc_name="A20";break;
- case 0x1650: soc_name="A23";break;
- case 0x1639: soc_name="A80";break;
+ case 0x1623: soc_name="A10"; break;
+ case 0x1625: soc_name="A13"; break;
+ case 0x1633: soc_name="A31"; break;
+ case 0x1639: soc_name="A80"; break;
+ case 0x1650: soc_name="A23"; break;
+ case 0x1651: soc_name="A20"; break;
+ case 0x1667: soc_name="A33"; break;
+ case 0x1673: soc_name="A83T"; break;
+ case 0x1680: soc_name="H3"; break;
}

printf("%.8s soc=%08x(%s) %08x ver=%04x %02x %02x scratchpad=%08x %08x %08x\n",
@@ -942,8 +948,7 @@ int main(int argc, char **argv)
} else if (strncmp(argv[1], "dump", 4) == 0 && argc > 3) {
aw_fel_dump(handle, strtoul(argv[2], NULL, 0), strtoul(argv[3], NULL, 0));
skip = 3;
- } else if ((strncmp(argv[1], "exe", 3) == 0 && argc > 2)
- ) {
+ } else if ((strncmp(argv[1], "exe", 3) == 0 && argc > 2)) {
aw_fel_execute(handle, strtoul(argv[2], NULL, 0));
skip=3;
} else if (strncmp(argv[1], "ver", 3) == 0 && argc > 1) {
--
2.0.5

[Bernhard Nortmann]

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

usb-boot | 116 +++++++++++++++++++++++++++++++++------------------------------
1 file changed, 61 insertions(+), 55 deletions(-)

diff --git a/usb-boot b/usb-boot
index 3881d29..be34004 100755
--- a/usb-boot
+++ b/usb-boot
@@ -20,72 +20,78 @@
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

-top=`dirname $0`
-if [ $# -lt 2 ]; then
- echo "Usage: $0 u-boot-spl.bin u-boot.bin [boot.scr] [kernel script.bin [initramfs]]"
+# default (write) addresses, normally should match your U-Boot version
+kernel_addr_r=0x42000000 # kernel
+fdt_addr_r=0x43000000 # .dtb or script.bin
+scriptaddr=0x43100000 # boot script (.scr)
+ramdisk_addr_r=0x43300000 # initramfs
+
+usage() {
+ echo
+ echo "Usage:"
+ echo "$0 <spl> [<boot.scr> [<kernel> <devicefile> [<initramfs>]]]"
+ echo
+ echo "<spl> is typically u-boot-sunxi-with-spl.bin,"
+ echo "<devicefile> is script.bin (for older 3.4.x kernel),"
+ echo "or a .dtb file for newer (mainline) kernel using device tree"
+ echo
exit 1
-fi
-board=$1; shift || (echo "ERROR: u-boot-spl.bin must be specified"; exit 1;)
-uboot=$1; shift || (echo "ERROR: u-boot.bin must be specified"; exit 1;)
-bootscr=$top/felboot/ramboot.scr
-if [ ! -f $bootscr ]; then
- bootscr=$top/bin/ramboot.scr
-fi
-case "$1" in
-*.scr) bootscr="$1"; shift
- ;;
-esac
-if [ $# -ge 1 ]; then
- kernel=$1; shift || true
-fi
-if [ $# -ge 1 ]; then
- scriptbin=$1; shift || true
-fi
-if [ $# -ge 1 ]; then
- initramfs=$1; shift || true
-fi
+}
+
+top=`dirname $0`
fel() {
+ # echo fel command (args) to console, then execute it
echo fel "$@"
$top/fel $@
}
-case $board in
-*/*) felboot=$board
- ;;
-*)
- felboot=$top/felboot/fel-boot-${board}.bin
- if [ ! -f $felboot ]; then
- felboot=$top/bin/fel-boot-${board}.bin
+felfile() {
+ # (optionally) expand a filename ($2) with default directory,
+ # and ensure a corresponding file exists
+ # returns (possibly modified) filename as environment var specified by $1
+ fname=$2
+ case ${fname} in
+ */*) ;; # use any filename with slashes 'as-is'
+ *) fname="${top}/felboot/$2" # prefix with standard dir
+ if [ ! -f ${fname} ]; then
+ fname="${top}/bin/$2" # try "bin" dir instead
+ fi
+ ;;
+ esac
+ if [ ! -f ${fname} ]; then
+ echo "ERROR: ($1) file ${fname} not found"
+ exit 1
fi
- ;;
-esac
-if [ ! -f $felboot ]; then
- echo "ERROR: Can't find SPL FEL binary ${board}"
- exit 1
+ export $1="${fname}"
+}
+
+if [ $# -lt 1 ]; then usage; fi
+
+felfile "spl" $1; shift || (echo "ERROR: <spl> must be specified"; usage;)
+
+if [ $# -ge 1 ]; then
+ felfile "bootscr" $1; shift
fi
-if [ `wc -c $felboot | cut '-d ' -f1` -gt 15616 ]; then
- echo "ERROR: SPL FEL binary too large. Must be the FEL version of SPL"
- exit 1
+if [ $# -ge 1 ]; then
+ felfile "kernel" $1; shift
fi
-if [ ! -f $bootscr ]; then
- echo "ERROR: Can't find boot script '${bootscr}'"
- exit 1
+if [ $# -ge 1 ]; then
+ felfile "scriptbin" $1; shift
fi
-fel write 0x2000 $felboot
-fel exe 0x2000
-sleep 1 # Wait for DRAM initialization to complete
-if [ -n "$uboot" ]; then
- fel write 0x4a000000 $uboot
+if [ $# -ge 1 ]; then
+ felfile "initramfs" $1; shift
fi
+
+FEL_ARGS="uboot $spl" # use "-v uboot $spl" for more verbose output
if [ -n "$bootscr" ]; then
- fel write 0x41000000 $bootscr
+ FEL_ARGS+=" write $scriptaddr $bootscr"
fi
if [ -n "$kernel" ]; then
- if [ -n "$scriptbin" ]; then
- fel write 0x43000000 $scriptbin
- fi
- fel write 0x44000000 $kernel
- if [ -n "$initramfs" ]; then
- fel write 0x4c000000 $initramfs
- fi
+ if [ -n "$scriptbin" ]; then
+ FEL_ARGS+=" write $fdt_addr_r $scriptbin"
+ fi
+ FEL_ARGS+=" write $kernel_addr_r $kernel"
+ if [ -n "$initramfs" ]; then
+ FEL_ARGS+=" write $ramdisk_addr_r $initramfs"
+ fi
fi
-fel exe 0x4a000000
+fel ${FEL_ARGS} # execute actual fel command
--
2.0.5

[Siarhei Siamashka]

Hi,

If anyone wonders, that's a sunxi-tools patch. To avoid confusion in
the future, it's best to have this information in the subject line.

Maybe it would be better to rework this code in order not assign a
special magical meaning of the negative 'len' value?

Either by passing the file name instead of the buffer pointer
and buffer len pair to the 'aw_fel_write_uboot_image' function.
Or by doing the check for missing u-boot part outside of the
'aw_fel_write_uboot_image'.

In the former case, we can also support fel booting via extracting
U-Boot from the SD card images (by checking the eGON signature both
in the beginning of the file and at a certain offset). And because
SD card images tend to be rather large (up to several gigabytes),
reading the whole file to RAM is not a very good idea.

> void aw_fel_write_uboot_image(libusb_device_handle *usb,
> - uint8_t *buf, size_t len)
> + uint8_t *buf, int len)
> {
> if (len <= HEADER_SIZE)
> return; /* Insufficient size (no actual data), just bail out */

If the user uses the 'uboot' command with improper file (without the
u-boot part), then he/she should probably see a descriptive error
message.

> @@ -774,17 +775,16 @@ void aw_fel_write_uboot_image(libusb_device_handle *usb,
> }
> uint32_t data_size = be32toh(buf32[3]); /* Image Data Size */
> uint32_t load_addr = be32toh(buf32[4]); /* Data Load Address */
> - if ((size_t)data_size != len - HEADER_SIZE) {
> + if ((int)data_size != len - HEADER_SIZE) {
> fprintf(stderr, "U-Boot image data size mismatch: "
> - "expected %d, got %u\n", (int)len - HEADER_SIZE,
> - data_size);
> + "expected %d, got %u\n", len - HEADER_SIZE, data_size);
> exit(1);
> }
> /* TODO: Verify image data integrity using the checksum field ih_dcrc,
> * available from be32toh(buf32[6])
> *
> * However, this requires CRC routines that mimic their U-Boot
> - * counterparts, namely image_check_dcrc() in ${U-BOOT}/common/image.cabs
> + * counterparts, namely image_check_dcrc() in ${U-BOOT}/common/image.c
> * and crc_wd() in ${U-BOOT}/lib/crc32.c
> *
> * It should be investigated if existing CRC routines in sunxi-tools



--

Best regards,
Siarhei Siamashka

[Siarhei Siamashka]

On Tue, 4 Aug 2015 01:26:45 +0200
Bernhard Nortmann <bernhard...@web.de> wrote:

Hi, thanks for the patch.

However currently the usb-boot script is primarily intended to work
with the the legacy https://github.com/linux-sunxi/u-boot-sunxi and
this is documented at http://linux-sunxi.org/FEL/USBBoot (albeit not
in a very obvious way). If we change the script to use the "fel uboot"
command, then the legacy u-boot-sunxi will stop working properly.

It would be nice to switch to the mainline u-boot and drop support
of the legacy u-boot-sunxi bootloader eventually, but not all the
boards are supported by the mainline u-boot yet.

It may be a better idea to just remove this script altogether.
And update the wiki to suggest using an old sunxi-tools tag
with the legacy u-boot-sunxi bootloader. And also update
the README file. So that the page at
https://github.com/linux-sunxi/sunxi-tools
does not mention 'usb-boot', but refers to
http://linux-sunxi.org/FEL/USBBoot
from the 'fel' section.

Now the command line interface of the 'fel' tool got much easier to
use with the addition of the "uboot" command. I'm not even sure if
we still want to use the wrapper script with the mainline U-Boot.

Best regards,
Siarhei Siamashka

[Ian Campbell]

On Tue, 2015-08-04 at 01:26 +0200, Bernhard Nortmann wrote:
> void aw_fel_write_uboot_image(libusb_device_handle *usb,
> - uint8_t *buf, size_t len)
> + uint8_t *buf, int len)

Siahei's comments may make this moot, but if you want len to be signed you
most likely want ssize_t.

Ian.

[Bernhard Nortmann]

Am 04.08.2015 10:33, schrieb Siarhei Siamashka:
> Hi,
>
> If anyone wonders, that's a sunxi-tools patch. To avoid confusion in
> the future, it's best to have this information in the subject line.

Yes, sorry - I forgot to add the "sunxi-tools" prefix, and also didn't
CC you as the maintainer.
Thanks for clarifying this.

> Maybe it would be better to rework this code in order not assign a
> special magical meaning of the negative 'len' value?
>
> Either by passing the file name instead of the buffer pointer
> and buffer len pair to the 'aw_fel_write_uboot_image' function.
> Or by doing the check for missing u-boot part outside of the
> 'aw_fel_write_uboot_image'.
>
> In the former case, we can also support fel booting via extracting
> U-Boot from the SD card images (by checking the eGON signature both
> in the beginning of the file and at a certain offset). And because
> SD card images tend to be rather large (up to several gigabytes),
> reading the whole file to RAM is not a very good idea.

Agreed. My intention for this 'magic' was to keep the call in
aw_fel_process_spl_and_uboot() down to a very simple
aw_fel_write_uboot_image(usb, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT).

But it's fair enough to simply test (size >= SPL_LEN_LIMIT) there, and to
skip aw_fel_write_uboot_image() entirely if the size is too small -
allowing us to stick with the more natural "size_t len" type.
Trying to limit aw_fel_write_uboot_image() to the actual handling of the
image (buffer) is a reasonable move, avoiding any 'magical' side effects.

> If the user uses the 'uboot' command with improper file (without the
> u-boot part), then he/she should probably see a descriptive error
> message.

The current implementation tries to take care of that by checking for the
presence of U-Boot after processing the file (and setting the
uboot_autostart
flag accordingly). If that fails for the "uboot" command invocation, a
warning
will be issued (Warning: "uboot" command failed to detect image!
Can't execute U-Boot.), and there will be no attempt to execute U-Boot.

Regards, B. Nortmann

[Siarhei Siamashka]

On Tue, 04 Aug 2015 15:10:41 +0200
Bernhard Nortmann <bernhard...@web.de> wrote:

> Am 04.08.2015 10:33, schrieb Siarhei Siamashka:
> > Hi,
> >
> > If anyone wonders, that's a sunxi-tools patch. To avoid confusion in
> > the future, it's best to have this information in the subject line.
>
> Yes, sorry - I forgot to add the "sunxi-tools" prefix, and also didn't
> CC you as the maintainer.
> Thanks for clarifying this.

I'm not a maintainer of sunxi-tools. I think that just the usual rules
apply. Every patch author needs to find at least one reviewer and get
an ack from him.

OK. You are right. Passing this information via global variables got me
confused. IMHO the code could have been a bit cleaner. But I guess,
this is not a big problem as long as the fel tool source code is still
so small.

[Bernhard Nortmann]

Hello Siarhei!

Am 13.08.2015 09:43, schrieb Siarhei Siamashka:
>> [...]

> OK. You are right. Passing this information via global variables got me
> confused. IMHO the code could have been a bit cleaner. But I guess,
> this is not a big problem as long as the fel tool source code is still
> so small.

Yes, that's bit awkward. I've reexamined it and at least the
"uboot_autostart" flag could be moved into the main() scope. We still
need it to preserve its value across several commands, as "uboot" will
possibly be followed by other operations, before the final
aw_fel_execute() takes place. "uboot_entry" and "uboot_size" have to be
global to fulfill their function as safeguards for aw_fel_write().

I've prepared and attached a new patch, which also includes a
conditional statement to solve the underflow issue in a clean way.

Regards, B. Nortmann

[Siarhei Siamashka]

I guess, this patch is now superseded by the recent patches from Vishnu
Patekar, which add full support for H3, A83T and A33.

[Siarhei Siamashka]

Thanks!

> From 4bf3111f73ec3f39e0cc0565f172013a5497b4d0 Mon Sep 17 00:00:00 2001
> From: Bernhard Nortmann <bernhard...@web.de>
> Date: Mon, 17 Aug 2015 18:57:36 +0200
> Subject: [PATCH 1/3 v2] sunxi-tools: Prevent signed values (fix possible
> underflow) of "len" parameter for aw_fel_write_uboot_image(), and narrow down
> scope for uboot_autostart

Maybe I'm nitpicking, but the subject line looks excessively long
here. Maybe because the patch fixes two unrelated things and
describes both of them in the subject line?

Regarding the commit message in general, it might be a good idea
to describe the user visible symptoms of the bug too. For example,
maybe mention that the out of bounds memory accesses can happen
when running "fel uboot sunxi-spl.bin" (when sunxi-spl.bin is
smaller than 32K)?

> Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
> ---

> fel.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fel.c b/fel.c
> index 41b19c9..e36e92d 100644
> --- a/fel.c
> +++ b/fel.c
> @@ -64,7 +64,6 @@ static int timeout = 60000;
> static int verbose = 0; /* Makes the 'fel' tool more talkative if non-zero */
> static uint32_t uboot_entry = 0; /* entry point (address) of U-Boot */
> static uint32_t uboot_size = 0; /* size of U-Boot binary */
> -static int uboot_autostart = 0; /* "uboot" command flag = autostart U-Boot */
>
> static void pr_info(const char *fmt, ...)
> {
> @@ -815,7 +814,8 @@ void aw_fel_process_spl_and_uboot(libusb_device_handle *usb,
> /* write and execute the SPL from the buffer */
> aw_fel_write_and_execute_spl(usb, buf, size);
> /* check for optional main U-Boot binary (and transfer it, if applicable) */
> - aw_fel_write_uboot_image(usb, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT);
> + if (size > SPL_LEN_LIMIT)
> + aw_fel_write_uboot_image(usb, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT);

That's a simple fix and I like it better than converting 'size_t' to 'int'
in more than one place and then dealing with the negative size inside of
the 'aw_fel_write_uboot_image()' function. Looks good to me.

> }
>
> static int aw_fel_get_endpoint(libusb_device_handle *usb)
> @@ -868,6 +868,7 @@ static double gettime(void)

>
> int main(int argc, char **argv)

> {
> + int uboot_autostart = 0; /* flag for "uboot" command = U-Boot autostart */
> int rc;
> libusb_device_handle *handle = NULL;
> int iface_detached = -1;
> --
> 2.0.5

Making "uboot_autostart" a local variable instead of global looks
like a good change too. It would look great as a standalone patch.

[Bernhard Nortmann]

Hi Siarhei!

Those are some good arguments. I have prepared two separate patches
to follow up on this, which should apply nicely/easily.

Regards, B. Nortmann

[Bernhard Nortmann]

Am 19.08.2015 17:07, schrieb Siarhei Siamashka:
> I guess, this patch is now superseded by the recent patches from
> Vishnu Patekar, which add full support for H3, A83T and A33.

Full ACK. The patch was just there to introduce a better SoC ID
identification
for end user's convenience.

I'd still prefer to have the cases sorted, but that's something that you and
Vishnu should discuss / agree on.

Regards, B. Nortmann

[Bernhard Nortmann]

Hello Siarhei!

We've been discussing that usb-boot script (and the Wiki stuff related)
on IRC.
I agree that it's pretty much obsolete now that "fel uboot" gracefully
handles
most of these tasks. You voted for (and I agreed on) removing the script in
the near future, and only reference it for the purpose of supporting and/or
documenting the legacy u-boot-sunxi, possibly using an archived or tagged
revision of sunxi-tools.

Regards, B. Nortmann

[Siarhei Siamashka]

On Thu, 20 Aug 2015 14:20:34 +0200
Bernhard Nortmann <bernhard...@web.de> wrote:

> Hello Siarhei!
>
> We've been discussing that usb-boot script (and the Wiki stuff related)
> on IRC.
> I agree that it's pretty much obsolete now that "fel uboot" gracefully
> handles
> most of these tasks. You voted for (and I agreed on) removing the script in
> the near future,

Yes, just sent a patch for the 'usb-boot' script removal to the
mailing list.

> and only reference it for the purpose of supporting and/or
> documenting the legacy u-boot-sunxi, possibly using an archived or tagged
> revision of sunxi-tools.

Now this is also done:
http://linux-sunxi.org/index.php?title=FEL/USBBoot&curid=219&diff=14820&oldid=14819