LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* RE: [PATCH v2 43/63] net: qede: Use memset_startat() for counters
@ 2021-08-24 16:51 Shai Malin
  0 siblings, 0 replies; 2+ messages in thread
From: Shai Malin @ 2021-08-24 16:51 UTC (permalink / raw)
  To: Kees Cook, linux-kernel
  Cc: Ariel Elior, GR-everest-linux-l2, David S. Miller,
	Jakub Kicinski, netdev, Gustavo A. R. Silva, Greg Kroah-Hartman,
	Andrew Morton, linux-wireless, dri-devel, linux-staging,
	linux-block, linux-kbuild, clang-built-linux, Rasmus Villemoes,
	linux-hardening

On August 18, 2021 9:05 AM -0300, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memset(), avoid intentionally writing across
> neighboring fields.
> 
> Use memset_startat() so memset() doesn't get confused about writing
> beyond the destination member that is intended to be the starting point
> of zeroing through the end of the struct.
> 
> The old code was doing the wrong thing: it starts from the second member
> and writes beyond int_info, clobbering qede_lock:
> 
> struct qede_dev {
> 	...
>         struct qed_int_info             int_info;
> 
>         /* Smaller private variant of the RTNL lock */
>         struct mutex                    qede_lock;
> 	...
> 
> struct qed_int_info {
>         struct msix_entry       *msix;
>         u8                      msix_cnt;
> 
>         /* This should be updated by the protocol driver */
>         u8                      used_cnt;
> };
> 
> Cc: Ariel Elior <aelior@marvell.com>
> Cc: GR-everest-linux-l2@marvell.com
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/net/ethernet/qlogic/qede/qede_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c
> b/drivers/net/ethernet/qlogic/qede/qede_main.c
> index d400e9b235bf..0ed9a0c8452c 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> @@ -2419,7 +2419,7 @@ static int qede_load(struct qede_dev *edev, enum
> qede_load_mode mode,
>  	goto out;
>  err4:
>  	qede_sync_free_irqs(edev);
> -	memset(&edev->int_info.msix_cnt, 0, sizeof(struct qed_int_info));
> +	memset_startat(&edev->int_info, 0, msix_cnt);

As I commented on V1:
"[PATCH 42/64] net: qede: Use memset_after() for counters",
the memset is redundant and it should clear only the msix_cnt.

We will fix it.

>  err3:
>  	qede_napi_disable_remove(edev);
>  err2:
> --
> 2.30.2


^ permalink raw reply	[flat|nested] 2+ messages in thread

* [PATCH v2 43/63] net: qede: Use memset_startat() for counters
  2021-08-18  6:04 [PATCH v2 00/63] Introduce strict memcpy() bounds checking Kees Cook
@ 2021-08-18  6:05 ` Kees Cook
  0 siblings, 0 replies; 2+ messages in thread
From: Kees Cook @ 2021-08-18  6:05 UTC (permalink / raw)
  To: linux-kernel
  Cc: Kees Cook, Ariel Elior, GR-everest-linux-l2, David S. Miller,
	Jakub Kicinski, netdev, Gustavo A. R. Silva, Greg Kroah-Hartman,
	Andrew Morton, linux-wireless, dri-devel, linux-staging,
	linux-block, linux-kbuild, clang-built-linux, Rasmus Villemoes,
	linux-hardening

In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.

Use memset_startat() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.

The old code was doing the wrong thing: it starts from the second member
and writes beyond int_info, clobbering qede_lock:

struct qede_dev {
	...
        struct qed_int_info             int_info;

        /* Smaller private variant of the RTNL lock */
        struct mutex                    qede_lock;
	...

struct qed_int_info {
        struct msix_entry       *msix;
        u8                      msix_cnt;

        /* This should be updated by the protocol driver */
        u8                      used_cnt;
};

Cc: Ariel Elior <aelior@marvell.com>
Cc: GR-everest-linux-l2@marvell.com
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/net/ethernet/qlogic/qede/qede_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
index d400e9b235bf..0ed9a0c8452c 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_main.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
@@ -2419,7 +2419,7 @@ static int qede_load(struct qede_dev *edev, enum qede_load_mode mode,
 	goto out;
 err4:
 	qede_sync_free_irqs(edev);
-	memset(&edev->int_info.msix_cnt, 0, sizeof(struct qed_int_info));
+	memset_startat(&edev->int_info, 0, msix_cnt);
 err3:
 	qede_napi_disable_remove(edev);
 err2:
-- 
2.30.2


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-08-24 16:52 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24 16:51 [PATCH v2 43/63] net: qede: Use memset_startat() for counters Shai Malin
  -- strict thread matches above, loose matches on Subject: below --
2021-08-18  6:04 [PATCH v2 00/63] Introduce strict memcpy() bounds checking Kees Cook
2021-08-18  6:05 ` [PATCH v2 43/63] net: qede: Use memset_startat() for counters Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).