Re: [PATCH] ICMP extensions for MPLS support for traceroute(8)

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

:+	 data_len = ip_len - ((u_char *)cmn_hdr - (u_char *)ip);
:+
:...
:+	 buf += sizeof(*cmn_hdr);
:+	 data_len -= sizeof(*cmn_hdr);
:+
:+	 while (data_len > 0) {
:+		obj_hdr = (struct icmp_ext_obj_hdr *)buf;
:+		obj_len = ntohs(obj_hdr->length);

    This should be data_len >= sizeof(struct icmp_ext_obj_hdr), 
    not >= 0.
	
	 while (data_len >= sizeof(struct icmp_ext_obj_hdr)) {
		...

:+
:+		/*
:+		 * Sanity check the length field
:+		 */
:+		if (obj_len > data_len) {
:+			return;
:+		}

    obj_len can be 0.  Check that obj_len < sizeof(*obj_hdr)
    and return if it isn't.  obj_len can be an odd number, which
    is also not a good idea.  Note sure about alignment requirements,
    it might even have to be 4-byte aligned.  But 2-byte for sure.

		if (obj_len < sizeof(*obj_hdr) || obj_len > data_len)
			return;
		if (obj_len & 3)	/* either & 1 or & 3, depending */
			return;

:+
:+		data_len -= obj_len;
:+
:+		/*
:+		 * Move past the object header
:+		 */
:+		buf += sizeof(struct icmp_ext_obj_hdr);
:+		obj_len -= sizeof(struct icmp_ext_obj_hdr);
:+
:+		switch (obj_hdr->class_num) {
:+		case MPLS_STACK_ENTRY_CLASS:
:+			switch (obj_hdr->c_type) {
:+			case MPLS_STACK_ENTRY_C_TYPE:
:+				while (obj_len >= (int)sizeof(uint32_t)) {
:+					mpls_hdr = ntohl(*(uint32_t *)buf);
:+
:+					buf += sizeof(uint32_t);
:+					obj_len -= sizeof(uint32_t);
:+					printf(" [MPLS: Label %d Exp %d]",
:+					    MPLS_LABEL(mpls_hdr), MPLS_EXP(mpls_hdr));
:+				}
:+				if (obj_len > 0) {
:+					/*
:+					 * Something went wrong, and we're at
:+					 * a unknown offset into the packet,
:+					 * ditch the rest of it.
:+					 */
:+					return;
:+				}
:+				break;
:+			default:
:+				/*
:+				 * Unknown object, skip past it
:+				 */
:+				buf += ntohs(obj_hdr->length) -
:+				    sizeof(struct icmp_ext_obj_hdr);

    Hmm.  This is a bit messy

:...
:+			/*
:+			 * Unknown object, skip past it
:+			 */
:+			buf += ntohs(obj_hdr->length) -
:+			    sizeof(struct icmp_ext_obj_hdr);

    Same... a bit messy.

    I suggest declaring a 'nextbuf' which you calculate up at the
    'Move past the object header' code and just assign buf = nextbuf
    in these two places.

:+			break;
:+		}
:+	}
: }

					-Matt
					Matthew Dillon 
					<dillon@backplane.com>