Re: [PATCH crypto 2/2] crypto: caam - add allocation failure handling in SPRINTFCAT macro

[Marek Vasut <marex@denx.de>]

On Wednesday, April 23, 2014 at 07:12:19 PM, Marek Vasut wrote:
> 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 ?

Ok, I apologize. You were right. The 'strcat()' is always called with a fixed-
length 302byte long buffer allocated on stack. Thus this code is only fragile.

I will need to think of this code a bit more before I blurt out some serious 
nonsense again.

Best regards,
Marek Vasut