On Tue, Mar 23, 2021 at 4:57 PM Rasmus Villemoes linux@rasmusvillemoes.dk wrote:
On 23/03/2021 14.04, Arnd Bergmann wrote:
if (securedisplay_cmd->status == TA_SECUREDISPLAY_STATUS__SUCCESS) {
int pos = 0; memset(i2c_output, 0, sizeof(i2c_output)); for (i = 0; i < TA_SECUREDISPLAY_I2C_BUFFER_SIZE; i++)
sprintf(i2c_output, "%s 0x%X", i2c_output,
pos += sprintf(i2c_output + pos, " 0x%X", securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]); dev_info(adev->dev, "SECUREDISPLAY: I2C buffer out put is :%s\n", i2c_output);Eh, why not get rid of the 256 byte stack allocation and just replace all of this by
dev_info(adev->dev, ""SECUREDISPLAY: I2C buffer out put is: %*ph\n", TA_SECUREDISPLAY_I2C_BUFFER_SIZE, securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf);
That's much less code (both in #LOC and .text), and avoids adding yet another place that will be audited over and over for "hm, yeah, that sprintf() is actually not gonna overflow".
Yeah, it'll lose the 0x prefixes for each byte and use lowercase hex chars.
Ah, I didn't know the kernel's sprintf could do that, that's really nice.
I'll send a follow-up patch, as Alex has already applied the first one.
Alex, feel free to merge the two into one, or just keep as they are.
Arnd