| Message ID | 523F979C.1070205@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfgang Denk |
| Headers | show |
On Sun, Sep 22, 2013 at 10:21 PM, Steven Falco <stevenfalco@gmail.com> wrote: > Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. > In this case, there will be a null cmdtp pointer, and we must not > dereference > it. > > Signed-off-by: Steven A. Falco <stevenfalco@gmail.com> > > --- Acked-by: Otavio Salvador <otavio@ossystems.com.br> Tom, will you pick this or should it be Cced to someone specific?
Hi Steven, On Sun, 22 Sep 2013 21:21:32 -0400, Steven Falco <stevenfalco@gmail.com> wrote: > Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. > In this case, there will be a null cmdtp pointer, and we must not dereference > it. > > Signed-off-by: Steven A. Falco <stevenfalco@gmail.com> > > --- > > In file cmd_pxe.c around line 687 is a call: > > do_bootm(NULL, 0, bootm_argc, bootm_argv); > > Notice that the first argument is NULL. Therefore, the cmdtp > pointer will always be NULL when using the pxe boot mechanism. > > do_bootm() eventually calls boot_get_kernel(), still with cmdtp == NULL. > In the Wandboard case, the vmlinuz binary is not "legacy format", nor is > it "fit format", so U-Boot attempts to print: > > printf("Wrong Image Format for %s command\n", cmdtp->name); > > That is doomed to fail, because cmdtp is NULL. The following patch corrects > the problem; the command name will be printed only if the pointer is valid. Then shouldn't the patch subject/summary be "print command name only if cmdtp is not NULL" rather than the quite uninformative "prevent a crash"? Amicalement,
On Mon, Sep 23, 2013 at 1:11 PM, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Then shouldn't the patch subject/summary be "print command name only > if cmdtp is not NULL" rather than the quite uninformative "prevent a > crash"? Yes, I agree that original subject is a bit misleading. When I read it I thought it was a Wandboard related problem. Regards, Fabio Estevam
On Mon, Sep 23, 2013 at 01:21:34PM -0300, Fabio Estevam wrote: > On Mon, Sep 23, 2013 at 1:11 PM, Albert ARIBAUD > <albert.u.boot@aribaud.net> wrote: > > > Then shouldn't the patch subject/summary be "print command name only > > if cmdtp is not NULL" rather than the quite uninformative "prevent a > > crash"? > > Yes, I agree that original subject is a bit misleading. I'm re-wording the commit message now, just got side-tracked fixing another problem I introduced on Friday :(
Dear Steven Falco, In message <523F979C.1070205@gmail.com> you wrote: > Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. > In this case, there will be a null cmdtp pointer, and we must not dereference > it. ... > - printf("Wrong Image Format for %s command\n", cmdtp->name); > + if (cmdtp) > + printf("Wrong Image Format for %s command\n", cmdtp->name); > + else > + printf("Wrong Image Format for command\n"); This is the wrong way to fix it. Instead of handling this here, please fix the place where a NULL pointer is passed incorrectly. Also, the error message "Wrong Image Format for command" makes no sense and gives no help to the user to understand what's wrong. NAK. Best regards, Wolfgang Denk
Dear Otavio Salvador, In message <CAP9ODKrdjdph+8mrXOf+zOjzL3Qs1U9k5kaDey896fOO_CLfWw@mail.gmail.com> you wrote: > On Sun, Sep 22, 2013 at 10:21 PM, Steven Falco <stevenfalco@gmail.com> wrote: > > Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. > > In this case, there will be a null cmdtp pointer, and we must not > > dereference > > it. > > > > Signed-off-by: Steven A. Falco <stevenfalco@gmail.com> > > > > --- > > Acked-by: Otavio Salvador <otavio@ossystems.com.br> > > Tom, will you pick this or should it be Cced to someone specific? Please don't. This should be fixed at the root of the problem instead. And in no case a bogus error message should be printed. Best regards, Wolfgang Denk
Dear Fabio Estevam, In message <CAOMZO5Aj56KVTfRqBd3Wq7OiP8q3wA0Yv4evUzerkXFuFvxvHQ@mail.gmail.com> you wrote: > > > Then shouldn't the patch subject/summary be "print command name only > > if cmdtp is not NULL" rather than the quite uninformative "prevent a > > crash"? > > Yes, I agree that original subject is a bit misleading. > > When I read it I thought it was a Wandboard related problem. I don't know if it's only Wandboard, or if other boards are affected, too (which are these? under which exact test cases?). In any case. the problem is not here, but on the caller's side. It should not call a function which expects a command name with a NULL pointer passed as argument. Best regards, Wolfgang Denk
On Mon, Sep 23, 2013 at 08:50:55PM +0200, Wolfgang Denk wrote: > Dear Fabio Estevam, > > In message <CAOMZO5Aj56KVTfRqBd3Wq7OiP8q3wA0Yv4evUzerkXFuFvxvHQ@mail.gmail.com> you wrote: > > > > > Then shouldn't the patch subject/summary be "print command name only > > > if cmdtp is not NULL" rather than the quite uninformative "prevent a > > > crash"? > > > > Yes, I agree that original subject is a bit misleading. > > > > When I read it I thought it was a Wandboard related problem. > > I don't know if it's only Wandboard, or if other boards are affected, > too (which are these? under which exact test cases?). In any case. > the problem is not here, but on the caller's side. It should not call > a function which expects a command name with a NULL pointer passed as > argument. I looked around at this a bit this morning. cmd_pxe.c would need a lot of mangling to pass around cmdtp, just for the sake of an error message that's then ignored as the caller logic is: 1) Try bootm on the image 2) If CONFIG_CMD_BOOTZ, if bootm returned, maybe we got a zImage? do_bootz instead (also NULL cmdtp). The error message wouldn't exactly make sense here either, being invoked via menu.
On Mon, Sep 23, 2013 at 03:17:57PM -0400, Tom Rini wrote: > On Mon, Sep 23, 2013 at 08:50:55PM +0200, Wolfgang Denk wrote: > > > Dear Fabio Estevam, > > > > In message <CAOMZO5Aj56KVTfRqBd3Wq7OiP8q3wA0Yv4evUzerkXFuFvxvHQ@mail.gmail.com> you wrote: > > > > > > > Then shouldn't the patch subject/summary be "print command name only > > > > if cmdtp is not NULL" rather than the quite uninformative "prevent a > > > > crash"? > > > > > > Yes, I agree that original subject is a bit misleading. > > > > > > When I read it I thought it was a Wandboard related problem. > > > > I don't know if it's only Wandboard, or if other boards are affected, > > too (which are these? under which exact test cases?). In any case. > > the problem is not here, but on the caller's side. It should not call > > a function which expects a command name with a NULL pointer passed as > > argument. > > I looked around at this a bit this morning. cmd_pxe.c would need a lot > of mangling to pass around cmdtp, just for the sake of an error message > that's then ignored as the caller logic is: > 1) Try bootm on the image > 2) If CONFIG_CMD_BOOTZ, if bootm returned, maybe we got a zImage? > do_bootz instead (also NULL cmdtp). > > The error message wouldn't exactly make sense here either, being invoked > via menu. So, I went off an modified cmd_pxe.c to pass around cmdtp so that we get an error message out, and it's not too bad looking, but it highlights another problem, which is that we could really use a way to get at least the "is this a ... ?" code, and just get the error code, rather than printouts. The pxe (and really it's the syslinux.conf/extlinux.conf parsing) code shouldn't be doing bootm();bootz() but checking the image type and calling the right boot.
Dear Tom, In message <20130923191757.GZ5273@bill-the-cat> you wrote: > > > I don't know if it's only Wandboard, or if other boards are affected, > > too (which are these? under which exact test cases?). In any case. > > the problem is not here, but on the caller's side. It should not call > > a function which expects a command name with a NULL pointer passed as > > argument. > > I looked around at this a bit this morning. cmd_pxe.c would need a lot > of mangling to pass around cmdtp, just for the sake of an error message > that's then ignored as the caller logic is: > 1) Try bootm on the image > 2) If CONFIG_CMD_BOOTZ, if bootm returned, maybe we got a zImage? > do_bootz instead (also NULL cmdtp). > > The error message wouldn't exactly make sense here either, being invoked > via menu. To me that meens that the whole logic needs rework. We should never just "try out" if an image is in uImage format or a zImage - we are perfectly able to detect such information from the file header (in case of uImage at least). If we keep the code as is, what's wrong then with using the pxe command as ID string? Then the end user knows at least that it was this command which was failing (or producing the message). Best regards, Wolfgang Denk
Dear Tom, In message <20130923194554.GA5273@bill-the-cat> you wrote: > > So, I went off an modified cmd_pxe.c to pass around cmdtp so that we get > an error message out, and it's not too bad looking, but it highlights > another problem, which is that we could really use a way to get at least > the "is this a ... ?" code, and just get the error code, rather than > printouts. The pxe (and really it's the syslinux.conf/extlinux.conf > parsing) code shouldn't be doing bootm();bootz() but checking the image > type and calling the right boot. Ah, full ACK (I should have read the thread to end before posting). Thanks! Best regards, Wolfgang Denk
On 09/23/2013 04:44 PM, Wolfgang Denk wrote: > Dear Tom, > > In message <20130923194554.GA5273@bill-the-cat> you wrote: >> >> So, I went off an modified cmd_pxe.c to pass around cmdtp so that we get >> an error message out, and it's not too bad looking, but it highlights >> another problem, which is that we could really use a way to get at least >> the "is this a ... ?" code, and just get the error code, rather than >> printouts. The pxe (and really it's the syslinux.conf/extlinux.conf >> parsing) code shouldn't be doing bootm();bootz() but checking the image >> type and calling the right boot. > > Ah, full ACK (I should have read the thread to end before posting). > > Thanks! > > Best regards, > > Wolfgang Denk > I understand your point that it is better to fix the problem at the source. I also like Tom's suggestion that the type be checked earlier, and then just directly choose the right bootX() variant. So naturally, I withdraw my patch, in favor of your fix - at least I was able to isolate the source of the crash for you. :-) Steve
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 349f165..2249682 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -985,7 +985,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, break; #endif default: - printf("Wrong Image Format for %s command\n", cmdtp->name); + if (cmdtp) + printf("Wrong Image Format for %s command\n", cmdtp->name); + else + printf("Wrong Image Format for command\n"); bootstage_error(BOOTSTAGE_ID_FIT_KERNEL_INFO); return NULL; }
Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. In this case, there will be a null cmdtp pointer, and we must not dereference it. Signed-off-by: Steven A. Falco <stevenfalco@gmail.com> --- In file cmd_pxe.c around line 687 is a call: do_bootm(NULL, 0, bootm_argc, bootm_argv); Notice that the first argument is NULL. Therefore, the cmdtp pointer will always be NULL when using the pxe boot mechanism. do_bootm() eventually calls boot_get_kernel(), still with cmdtp == NULL. In the Wandboard case, the vmlinuz binary is not "legacy format", nor is it "fit format", so U-Boot attempts to print: printf("Wrong Image Format for %s command\n", cmdtp->name); That is doomed to fail, because cmdtp is NULL. The following patch corrects the problem; the command name will be printed only if the pointer is valid.