[Openswan dev] ARM unaligned bug

[D. Hugh Redelmeier]

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

| So I compiled in debug information and traced it with GDB to the
| function out_modify_previous_np() in lib/libpluto/packet.c. The inner
| loop (begins on line 1675) looks like this:
| 
| ...
| struct isakmp_generic *hdr;
| for (offset = sizeof(struct isakmp_hdr); offset < len ;
| 	offset += ntohs(hdr->isag_length)) {
| 	if ((len - offset) < sizeof(struct isakmp_generic))
| 		return FALSE;
| 	hdr = (struct isakmp_generic *)(outs->start+offset);
| 	if ((len - offset) < ntohs(hdr->isag_length))
| 		return FALSE;
| 	if ((len - offset) == ntohs(hdr->isag_length)) {
| 		hdr->isag_np = np;
| 		return TRUE;
| 	}
| }
| ...
| 
| I couldn't beleive my eyes, but the problem was hdr->isag_length. When
| (outs->start+offset) is on a non-aligned address ARM runs into trouble
| and for some reason returns 0 instead of the isag_length value.
| Because of that the for loop never terminates.

packet.c routines were carefully written to not depend on alignment.
Then this one was added.  As you point out, it is broken.

| My fix was to tell the compiler that struct isakmp_generic might point
| to a non aligned address:

That will do the job but that isn't the best way to do it.

Here's a version that attempts to do this right.  I have not even
compiled the code let alone tested it so it is surely wrong.

/* Find last complete top-level payload and change its np (or fail) */
bool
out_modify_previous_np(u_int8_t np, pb_stream *outs)
{
    size_t left = (outs->cur - outs->start);
    size_t offset = 0;

    for (;;) {
		if (left < sizeof(struct isakmp_hdr)) {
			return FALSE;	/* fail: no room for another payload */
		} else {
			size_t pllen = (outs->start[offset + offsetof(struct isakmp_generic, isag_length)] << 8)
			               | outs->start[offset + offsetof(struct isakmp_generic, isag_length) + 1]

			if (left < pllen) {
				return FALSE;	/* fail: not enough room for this payload */
			} else if (left == pllen) {
				/* found last top-level payload */
				outs->start[offset + offsetof(struct isakmp_generic, isag_np)] = np;
				return TRUE;
			} else {
				/* this payload is not the last: scan forward */
				offset += pllen;
				left -= pllen;
			}
		}
	}
}