scsi: 3w-sas: Fix unterminated strncpy
PATCH 1/1 scsi: 3w-sas: Fix unterminated strncpy
strncpy(dest, src, strlen(src)) leads to unterminated dest, which is dangerous.
Here driver_version's len is 32 and TW_DRIVER_VERSION is shorter than 32.
Therefore strcpy is OK.
--- drivers/scsi/3w-sas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/3w-sas.c +1 -1
@@ -1328,7 +1328,7 @@ static int twl_reset_sequence(TW_Device_Extension *tw_dev, int soft_reset) }/* Load rest of compatibility struct */ -strncpy (tw_dev->tw_compat_info.driver_version,TW_DRIVER_VERSION ,strlen (TW_DRIVER_VERSION )); +strcpy (tw_dev->tw_compat_info.driver_version,TW_DRIVER_VERSION ); tw_dev->tw_compat_info.driver_srl_high =TW_CURRENT_DRIVER_SRL ; tw_dev->tw_compat_info.driver_branch_high =TW_CURRENT_DRIVER_BRANCH ; tw_dev->tw_compat_info.driver_build_high =TW_CURRENT_DRIVER_BUILD ;
Signature · 1 line
2.20.1
Quoted 2 lines · Chuhong Yuan
strncpy(dest, src, strlen(src))leads to unterminated dest, which is dangerous.
I don't buy that. The structure is only used for the TW_IOCTL_GET_COMPATIBILITY_INFO ioctl and all the fields for that are fixed width and are copied over as such.
Quoted 3 lines
Here driver_version's len is 32 and
TW_DRIVER_VERSIONis shorter than 32.
Therefore strcpy is OK.
The best practice for copying a string to a fixed width destination that does get printed within the kernel would be what the 3w-9xxx.c does
strlcpy(tw_dev->tw_compat_info.driver_version, TW_DRIVER_VERSION, sizeof(tw_dev->tw_compat_info.driver_version));
But as I said, it doesn't really matter for a fixed width field that's never printed within the kernel.
James
Quoted 13 lines · James Bottomley
Quoted 2 lines · Chuhong Yuan
strncpy(dest, src, strlen(src))leads to unterminated dest, which is dangerous.I don't buy that. The structure is only used for the
TW_IOCTL_GET_COMPATIBILITY_INFOioctl and all the fields for that are fixed width and are copied over as such.Quoted 3 lines
Here driver_version's len is 32 and
TW_DRIVER_VERSIONis shorter than 32.
Therefore strcpy is OK.
Quoted 7 lines
The best practice for copying a string to a fixed width destination that does get printed within the kernel would be what the
3w-9xxx.cdoesstrlcpy(tw_dev->tw_compat_info.driver_version, TW_DRIVER_VERSION, sizeof(tw_dev->tw_compat_info.driver_version));
This is right, and strscpy() is better than strlcpy(). strlcpy() is deprecated now according to the documentation.
I choose strcpy() since it has better performance and there is no worry of overflow here. And I find there are indeed some places using strcpy() to fix this problem, like add_man_viewer() in tools/perf/builtin-help.c.
Quoted 3 lines
But as I said, it doesn't really matter for a fixed width field that's never printed within the kernel.
I think it is not good to leave a exploitable place here, and fixing it does not need much effort.
Regards,
Chuhong
Quoted 2 lines
James