On Mon, 17 Aug 2015 19:22:37 +0200 Bernhard Nortmann <[email protected]> wrote:
> 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. Thanks! > From 4bf3111f73ec3f39e0cc0565f172013a5497b4d0 Mon Sep 17 00:00:00 2001 > From: Bernhard Nortmann <[email protected]> > 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 <[email protected]> > --- > 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. -- Best regards, Siarhei Siamashka -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
