Thomas Graf wrote:

Dave,

This patch is actually part of a patchset for 2.6.11 inclusion
but the memory corruption possibility might make it worth
for 2.6.10. It's not really dangerous since it requires
admin capabilities though.

There are lots of places where this is possible, just look
at all the silly checks in the action code:

    sprintf(act_name, "%s", (char*)RTA_DATA(kind));
    if (RTA_PAYLOAD(kind) >= IFNAMSIZ) {

Puts the indev validation into its own function to allow
parameter validation before any changes are made. Changes
the sanity check from >= IFNAMSIZ to > IFNAMSIZ to correctly
handle 0 terminated strings and replaces the dangerous sprintf
with a memcpy bound to the TLV size. Providing a indev TLV
for kernels without CONFIG_NET_CLS_IND support will now lead
to EOPPNOTSUPP.

Why special-case indev ? Neither u32 nor fw make any attempt
to clean up after errors in their init functions, so instead
of fixing a single attribute they need to do proper cleanup,
than we can just continue to validate indev when changing it.
Returning EOPNOTSUPP makes sense, of course.

I'm also against keeping all those printks when touching the
code. Its ok when writing new code, but I don't see why this
code, unlike everything else, needs to report errors in the
ringbuffer instead of returning meaningful error codes.

+static inline void
+tcf_change_indev(struct tcf_proto *tp, char *indev, struct rtattr *id_tlv)
+{
+	memset(indev, 0, IFNAMSIZ);
+	memcpy(indev, RTA_DATA(id_tlv), RTA_PAYLOAD(id_tlv));
+	indev[IFNAMSIZ - 1] = '\0';
+}

And this should just use strlcpy.

Regards
Patrick