Re: [PATCH PKT_SCHED 4/17]: Check TCA_ACT_KIND payload size _before_ copying it

[Patrick McHardy <kaber@trash.net>]

Thomas Graf wrote:
> * Patrick McHardy <41D37875.5020103@trash.net> 2004-12-30 04:39
> 
>>-			sprintf(act_name, "%s", (char*)RTA_DATA(kind));
>>-			if (RTA_PAYLOAD(kind) >= IFNAMSIZ) {
>>-				printk("Action %s bad\n", (char*)RTA_DATA(kind));
>>+			if (RTA_PAYLOAD(kind) >= IFNAMSIZ)
> 
> 
> The check should be RTA_PAYLOAD(kind) > IFNAMSIZ, == is ok
> if the terminating NUL is provided.

Thanks.

> 
> 
>> 				goto err_out;
>>-			}
>>+			sprintf(act_name, "%s", (char*)RTA_DATA(kind));
>> 		} else {
> 
> 
> This will cause horrible crashes if no NUL is provided to terminate
> the name.
> 
> So I think this should be:
> 
> if (RTA_PAYLOAD(kind) > IFNAMSIZ)
> 	goto err_out;
> memset(act_name, ...);
> memcpy(act_name, RTA_DATA(kind), RTA_PAYLOAD(kind));
> act_name[IFNAMSIZ - 1] = '\0';
> 
> The memset is required to ensure 0 termination if kind is not and
> shorter than IFNAMSIZ. memcpy instead of str* to avoid using
> any form of str(n)len on a possibly not terminated string
> and setting IFNAMSIZ - 1 to NUL to ensure proper handling of
> a IFNAMSIZ long not terminated string.
> 
> I know it's unlikely but this might just save us some troubles later.

Agreed. I saved this change for later because there are more places
in net/sched that need to be fixed. I guess I'll just add a
rtattr_strncpy function.

Regards
Patrick