From: Arnd Bergmann arnd@arndb.de
gcc warns about an sprintf() that uses the same buffer as source and destination, which is undefined behavior in C99:
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c: In function 'amdgpu_securedisplay_debugfs_write': drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:141:6: error: 'sprintf' argument 3 overlaps destination object 'i2c_output' [-Werror=restrict] 141 | sprintf(i2c_output, "%s 0x%X", i2c_output, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 142 | securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:97:7: note: destination object referenced by 'restrict'-qualified argument 1 was declared here 97 | char i2c_output[256]; | ^~~~~~~~~~
Rewrite it to remember the current offset into the buffer instead.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c index 834440ab9ff7..69d7f6bff5d4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c @@ -136,9 +136,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC); if (!ret) { 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); } else {
Applied. Thanks!
Alex
On Tue, Mar 23, 2021 at 9:04 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
gcc warns about an sprintf() that uses the same buffer as source and destination, which is undefined behavior in C99:
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c: In function 'amdgpu_securedisplay_debugfs_write': drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:141:6: error: 'sprintf' argument 3 overlaps destination object 'i2c_output' [-Werror=restrict] 141 | sprintf(i2c_output, "%s 0x%X", i2c_output, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 142 | securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:97:7: note: destination object referenced by 'restrict'-qualified argument 1 was declared here 97 | char i2c_output[256]; | ^~~~~~~~~~
Rewrite it to remember the current offset into the buffer instead.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c index 834440ab9ff7..69d7f6bff5d4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c @@ -136,9 +136,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC); if (!ret) { 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); } else {-- 2.29.2
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 23/03/2021 14.04, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
gcc warns about an sprintf() that uses the same buffer as source and destination, which is undefined behavior in C99:
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c: In function 'amdgpu_securedisplay_debugfs_write': drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:141:6: error: 'sprintf' argument 3 overlaps destination object 'i2c_output' [-Werror=restrict] 141 | sprintf(i2c_output, "%s 0x%X", i2c_output, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 142 | securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:97:7: note: destination object referenced by 'restrict'-qualified argument 1 was declared here 97 | char i2c_output[256]; | ^~~~~~~~~~
Rewrite it to remember the current offset into the buffer instead.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c index 834440ab9ff7..69d7f6bff5d4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c @@ -136,9 +136,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC); if (!ret) { 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.
Rasmus
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
On 24/03/2021 14.33, Arnd Bergmann wrote:
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.
If you're bored, you can "git grep -E -C4 '%[0.]2[xX]'" and find places that are inside a small loop, many can trivially be converted to %ph, though often with some small change in formatting. If you're lucky, you even get to fix real bugs when people pass a "char" to %02x and "know" that that will produce precisely two bytes of output, so they've sized their stack buffer accordingly - boom when "char" happens to be signed and one of the bytes have a value beyond ascii and %02x produces 0xffffffXX.
%ph has a hard-coded upper bound of 64 bytes, I think that's silly because people instead do these inefficient and very verbose loops instead, wasting stack, .text and runtime.
Rasmus
On Tue, 2021-03-23 at 14:04 +0100, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
gcc warns about an sprintf() that uses the same buffer as source and destination, which is undefined behavior in C99:
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c: In function 'amdgpu_securedisplay_debugfs_write': drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:141:6: error: 'sprintf' argument 3 overlaps destination object 'i2c_output' [-Werror=restrict] 141 | sprintf(i2c_output, "%s 0x%X", i2c_output, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 142 | securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:97:7: note: destination object referenced by 'restrict'-qualified argument 1 was declared here 97 | char i2c_output[256]; | ^~~~~~~~~~
Rewrite it to remember the current offset into the buffer instead.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c index 834440ab9ff7..69d7f6bff5d4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c @@ -136,9 +136,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC); if (!ret) { 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);
Perhaps use a hex output like:
--- drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c index 9cf856c94f94..25bb34c72d20 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c @@ -97,13 +97,12 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u uint32_t op; int i; char str[64]; - char i2c_output[256]; int ret;
if (*pos || size > sizeof(str) - 1) return -EINVAL;
- memset(str, 0, sizeof(str)); + memset(str, 0, sizeof(str)); ret = copy_from_user(str, buf, size); if (ret) return -EFAULT; @@ -139,11 +138,9 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC); if (!ret) { if (securedisplay_cmd->status == TA_SECUREDISPLAY_STATUS__SUCCESS) { - 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, - 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); + dev_info(adev->dev, "SECUREDISPLAY: I2C buffer output is: %*ph\n", + (int)TA_SECUREDISPLAY_I2C_BUFFER_SIZE, + securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf); } else { psp_securedisplay_parse_resp_status(psp, securedisplay_cmd->status); }
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Arnd Bergmann -
Joe Perches -
Rasmus Villemoes