On Wednesday, April 23, 2014 at 06:35:45 PM, Horia Geantă wrote:
[...]
> > This entire macro looks somewhat strange.
>
> I am trying to fix it with minimal changes, so the patch qualifies for
> -stable.
This is just broken and you're not fixing it. You're just feeding this slimy
monster called technical debt more and more code, so it can grow and get uglier
and uglier. I hope you have no attachment to this abomination, since I'd like to
see it dead.
> > 1) Can't you just snprintf() into $str + some offset ? Something like:
> > snprintf(str + strlen(str), str_total_sz - strlen(str), format,
> > param);
>
> I think this would work. It also gets rid of memory allocation.
>
> Note that strlen(str) is undefined if str is not initialized /
> null-terminated.
> However, all code paths seem to touch this line in caam_jr_strstatus():
> sprintf(outstr, "%s: ", status_src[ssrc].error);
> before reaching SPRINTFCAT macros, so str is null-terminated.
>
> I'll send v2.
No, let us first agree on how to fix this insane abomination please.
But while I am looking, I see stuff like:
caam_jr_strstatus() can call report_ccb_status( , "CCB"); (basically with a
fixed-size string argument):
265 if (status_src[ssrc].report_ssed)
266 status_src[ssrc].report_ssed(status, outstr);
Report_ccb_status( , "CCB"); will call report_jump_idx( , "CCB"); (still with
fixed-size string arg), which contains your SPRINTFCAT() macro.
This will expand to:
...
strcat("CCB", tmp);
...
So basically you are writing into a fixed-size string? But the string is three-
bytes long, so you are overwriting kernel memory ?
If I read the code wrong, I really apologize in advance.
> > 2) Why is noone checking if the $str has enough space for contents of
> > $tmp ?
>
> All call sites reach this macro via caam_jr_strstatus(tmp, ...), which
> is always called having:
> char tmp[CAAM_ERROR_STR_MAX];
>
> CAAM_ERROR_STR_MAX is 302, probably enough according to commit
> de2954d66408da3ae34effda777bb564fd17781b (crypto: caam - fix printk
> recursion for long error texts).
You are digging in Linux's crypto code, not OpenSSL. We need accurate fixes and
accurate discussion ... using 'probably' does not work here.
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html