hinic: Replace memcpy() with direct assignment

Commit Message

Kees Cook June 16, 2022, 5:23 a.m. UTC

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" <[e-mail]>
Cc: Eric Dumazet <[e-mail]>
Cc: Jakub Kicinski <[e-mail]>
Cc: Paolo Abeni <[e-mail]>
Cc: Nathan Chancellor <[e-mail]>
Cc: Nick Desaulniers <[e-mail]>
Cc: Tom Rix <[e-mail]>
Cc: Leon Romanovsky <[e-mail]>
Cc: Jiri Pirko <[e-mail]>
Cc: Vladimir Oltean <[e-mail]>
Cc: Simon Horman <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Link: https://github.com/ClangBuiltLinux/linux/issues/1592
Signed-off-by: Kees Cook <[e-mail]>
---
 drivers/net/ethernet/huawei/hinic/hinic_devlink.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Gustavo A. R. Silva June 16, 2022, 10:43 a.m. UTC | #1

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" <[e-mail]>
> Cc: Eric Dumazet <[e-mail]>
> Cc: Jakub Kicinski <[e-mail]>
> Cc: Paolo Abeni <[e-mail]>
> Cc: Nathan Chancellor <[e-mail]>
> Cc: Nick Desaulniers <[e-mail]>
> Cc: Tom Rix <[e-mail]>
> Cc: Leon Romanovsky <[e-mail]>
> Cc: Jiri Pirko <[e-mail]>
> Cc: Vladimir Oltean <[e-mail]>
> Cc: Simon Horman <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Link: https://github.com/ClangBuiltLinux/linux/issues/1592
> Signed-off-by: Kees Cook <[e-mail]>

Reviewed-by: Gustavo A. R. Silva <[e-mail]>

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 ||

Nathan Chancellor June 16, 2022, 5:19 p.m. UTC | #2

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" <[e-mail]>
> Cc: Eric Dumazet <[e-mail]>
> Cc: Jakub Kicinski <[e-mail]>
> Cc: Paolo Abeni <[e-mail]>
> Cc: Nathan Chancellor <[e-mail]>
> Cc: Nick Desaulniers <[e-mail]>
> Cc: Tom Rix <[e-mail]>
> Cc: Leon Romanovsky <[e-mail]>
> Cc: Jiri Pirko <[e-mail]>
> Cc: Vladimir Oltean <[e-mail]>
> Cc: Simon Horman <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Link: https://github.com/ClangBuiltLinux/linux/issues/1592
> Signed-off-by: Kees Cook <[e-mail]>

Tested-by: Nathan Chancellor <[e-mail]> # 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
>

Gustavo A. R. Silva June 16, 2022, 10:26 p.m. UTC | #3

On 6/16/22 19:19, Nathan Chancellor wrote:

> Tested-by: Nathan Chancellor <[e-mail]> # build

I think in this case the tag should be "Build-tested-by" instead.

Thanks
--
Gustavo

[e-mail] June 17, 2022, 10:40 a.m. UTC | #4

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <[e-mail]>:

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!

[e-mail] June 22, 2022, 6:10 p.m. UTC | #5

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <[e-mail]>:

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!

Patch

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 ||