Under CONFIG_FORTIFY_SOURCE=y and CONFIG_UBSAN_BOUNDS=y, Clang is bugged
here for calculating the size of the destination buffer (0x10 instead of
0x14). This copy is a fixed size (sizeof(struct fw_section_info_st)), with
the source and dest being struct fw_section_info_st, so the memcpy should
be safe, assuming the index is within bounds, which is UBSAN_BOUNDS's
responsibility to figure out.
Avoid the whole thing and just do a direct assignment. This results in
no change to the executable code.
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Tom Rix <[email protected]>
Cc: Leon Romanovsky <[email protected]>
Cc: Jiri Pirko <[email protected]>
Cc: Vladimir Oltean <[email protected]>
Cc: Simon Horman <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: https://github.com/ClangBuiltLinux/linux/issues/1592
Signed-off-by: Kees Cook <[email protected]>
---
drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
index 60ae8bfc5f69..1749d26f4bef 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
@@ -43,9 +43,7 @@ static bool check_image_valid(struct hinic_devlink_priv *priv, const u8 *buf,
for (i = 0; i < fw_image->fw_info.fw_section_cnt; i++) {
len += fw_image->fw_section_info[i].fw_section_len;
- memcpy(&host_image->image_section_info[i],
- &fw_image->fw_section_info[i],
- sizeof(struct fw_section_info_st));
+ host_image->image_section_info[i] = fw_image->fw_section_info[i];
}
if (len != fw_image->fw_len ||
--
2.32.0
On 6/16/22 07:23, Kees Cook wrote:
> Under CONFIG_FORTIFY_SOURCE=y and CONFIG_UBSAN_BOUNDS=y, Clang is bugged
> here for calculating the size of the destination buffer (0x10 instead of
> 0x14). This copy is a fixed size (sizeof(struct fw_section_info_st)), with
> the source and dest being struct fw_section_info_st, so the memcpy should
> be safe, assuming the index is within bounds, which is UBSAN_BOUNDS's
> responsibility to figure out.
Also, there is a sanity check just before the for() loop:
38 if (fw_image->fw_info.fw_section_cnt > MAX_FW_TYPE_NUM) {
39 dev_err(&priv->hwdev->hwif->pdev->dev, "Wrong fw_type_num read from file, fw_type_num: 0x%x\n ",
40 fw_image->fw_info.fw_section_cnt);
41 return false;
42 }
so, this should be fine.
>
> Avoid the whole thing and just do a direct assignment. This results in
> no change to the executable code.
>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Tom Rix <[email protected]>
> Cc: Leon Romanovsky <[email protected]>
> Cc: Jiri Pirko <[email protected]>
> Cc: Vladimir Oltean <[email protected]>
> Cc: Simon Horman <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://github.com/ClangBuiltLinux/linux/issues/1592
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Gustavo A. R. Silva <[email protected]>
Thanks
--
Gustavo
> ---
> drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> index 60ae8bfc5f69..1749d26f4bef 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> @@ -43,9 +43,7 @@ static bool check_image_valid(struct hinic_devlink_priv *priv, const u8 *buf,
>
> for (i = 0; i < fw_image->fw_info.fw_section_cnt; i++) {
> len += fw_image->fw_section_info[i].fw_section_len;
> - memcpy(&host_image->image_section_info[i],
> - &fw_image->fw_section_info[i],
> - sizeof(struct fw_section_info_st));
> + host_image->image_section_info[i] = fw_image->fw_section_info[i];
> }
>
> if (len != fw_image->fw_len ||
On Wed, Jun 15, 2022 at 10:23:12PM -0700, Kees Cook wrote:
> Under CONFIG_FORTIFY_SOURCE=y and CONFIG_UBSAN_BOUNDS=y, Clang is bugged
> here for calculating the size of the destination buffer (0x10 instead of
> 0x14). This copy is a fixed size (sizeof(struct fw_section_info_st)), with
> the source and dest being struct fw_section_info_st, so the memcpy should
> be safe, assuming the index is within bounds, which is UBSAN_BOUNDS's
> responsibility to figure out.
>
> Avoid the whole thing and just do a direct assignment. This results in
> no change to the executable code.
>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Tom Rix <[email protected]>
> Cc: Leon Romanovsky <[email protected]>
> Cc: Jiri Pirko <[email protected]>
> Cc: Vladimir Oltean <[email protected]>
> Cc: Simon Horman <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://github.com/ClangBuiltLinux/linux/issues/1592
> Signed-off-by: Kees Cook <[email protected]>
Tested-by: Nathan Chancellor <[email protected]> # build
> ---
> drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> index 60ae8bfc5f69..1749d26f4bef 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> @@ -43,9 +43,7 @@ static bool check_image_valid(struct hinic_devlink_priv *priv, const u8 *buf,
>
> for (i = 0; i < fw_image->fw_info.fw_section_cnt; i++) {
> len += fw_image->fw_section_info[i].fw_section_len;
> - memcpy(&host_image->image_section_info[i],
> - &fw_image->fw_section_info[i],
> - sizeof(struct fw_section_info_st));
> + host_image->image_section_info[i] = fw_image->fw_section_info[i];
> }
>
> if (len != fw_image->fw_len ||
> --
> 2.32.0
>
On 6/16/22 19:19, Nathan Chancellor wrote:
> Tested-by: Nathan Chancellor <[email protected]> # build
I think in this case the tag should be "Build-tested-by" instead.
Thanks
--
Gustavo
Hello:
This patch was applied to netdev/net-next.git (master)
by David S. Miller <[email protected]>:
On Wed, 15 Jun 2022 22:23:12 -0700 you wrote:
> Under CONFIG_FORTIFY_SOURCE=y and CONFIG_UBSAN_BOUNDS=y, Clang is bugged
> here for calculating the size of the destination buffer (0x10 instead of
> 0x14). This copy is a fixed size (sizeof(struct fw_section_info_st)), with
> the source and dest being struct fw_section_info_st, so the memcpy should
> be safe, assuming the index is within bounds, which is UBSAN_BOUNDS's
> responsibility to figure out.
>
> [...]
Here is the summary with links:
- hinic: Replace memcpy() with direct assignment
https://git.kernel.org/netdev/net-next/c/2c0ab32b73cf
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <[email protected]>:
On Wed, 15 Jun 2022 22:23:12 -0700 you wrote:
> Under CONFIG_FORTIFY_SOURCE=y and CONFIG_UBSAN_BOUNDS=y, Clang is bugged
> here for calculating the size of the destination buffer (0x10 instead of
> 0x14). This copy is a fixed size (sizeof(struct fw_section_info_st)), with
> the source and dest being struct fw_section_info_st, so the memcpy should
> be safe, assuming the index is within bounds, which is UBSAN_BOUNDS's
> responsibility to figure out.
>
> [...]
Here is the summary with links:
- hinic: Replace memcpy() with direct assignment
https://git.kernel.org/netdev/net/c/1e70212e0315
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html