[Openswan dev] ARM unaligned bug

[D. Hugh Redelmeier]

| From: Albert Veli <albert.veli at gmail.com>

Thanks very much for finding these problems and testing proposed
solutions.

| If I do a couple of tiny modifications then it works on my two ARMs
| here. In include/packet.h it says size of isakmp_hdr is 20. But my GDB
| insists on 28 so I changed from
| 
| #define NSIZEOF_isakmp_hdr      20
| 
| to
| 
| #define NSIZEOF_isakmp_hdr      28
| 
| This works for the ARMs, but I don't know about other architectures.

Oops.  I read the size off a diagram in packet.h.  But the diagram
wasn't to scale.  Cookies are 8 bytes, not 4.  So I was off by 8.

Thanks!

| The other change was in packet.c. I commented out:
| 
| passert(pl[NOFFSETOF_isag_np] == ISAKMP_NEXT_NONE);
| 
| When I come to this line pl has the hex values: 0D 00 00 14,
| ISAKMP_NEXT_NONE is defined to 0. But 0xd is ISAKMP_NEXT_VID if I
| understand correctly. I am not familiar with the protocol, but that is
| what the debugger says.
| 
| If VID is optional (I'm speculating here), maybe this line could do:
| passert(pl[NOFFSETOF_isag_np] == ISAKMP_NEXT_NONE ||
| pl[NOFFSETOF_isag_np] == ISAKMP_NEXT_VID);

Interesting.

I added the asserts because I thought those conditions would be true,
not because I knew them to be true :-)

It seems to me that this routine is called to add a
previously-unexpected payload to a message.  So it would seem that the
last payload's "next payload" field should say ISAKMP_NEXT_NONE.  It
would be interesting to know why this isn't the case.

A slightly relaxed assertion that might work would be:
	passert(pl[NOFFSETOF_isag_np] == ISAKMP_NEXT_NONE
		|| pl[NOFFSETOF_isag_np] == np);
This says: the previous payload expected the next one to be nothing or
what we are going to provide.  I'm not saying that this is the right
one, just an interesting variant.

If it's not too much trouble, can you give us a traceback from a crash
where the original passert fails?  It might help me understand how
this happens.

Thanks again.