[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Steven Falco]

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.

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;
  	}

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Otavio Salvador]

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?

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Horses just naturally have mohawk haircuts.

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Fabio Estevam]

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

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Tom Rini]

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 :(

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130923/88ec65dc/attachment.pgp>

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The Buddha, the Godhead, resides quite as comfortably in the circuits
of a digital computer or the gears of a cycle transmission as he does
at the top of a mountain or in the petals of a flower.
            - R.  Pirsig, "Zen and the Art of Motorcycle Maintenance"

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130923/68df2ede/attachment.pgp>

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130923/3e0ca390/attachment.pgp>

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Here's a fish hangs in the net like a poor man's right in  the  law.
'Twill hardly come out."     - Shakespeare, Pericles, Act II, Scene 1

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Steven Falco]

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

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Plan to throw one away. You will anyway."
                              - Fred Brooks, "The Mythical Man Month"

[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Never worry about theory as long as  the  machinery  does  what  it's
supposed to do.                                      - R. A. Heinlein