Re: [PATCH 42/64] net: qede: Use memset_after() for counters
On Mon, Aug 02, 2021 at 02:29:28PM +, Shai Malin wrote: > > On Tue, Jul 31, 2021 at 07:07:00PM -0300, Kees Cook wrote: > > On Tue, Jul 27, 2021 at 01:58:33PM -0700, 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_after() 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. > > > > > > Signed-off-by: Kees Cook > > > --- > > > The old code seems to be doing the wrong thing: starting from not the > > > first member, but sized for the whole struct. Which is correct? > > > > Quick ping on this question. > > > > The old code seems to be doing the wrong thing: it starts from the second > > member and writes beyond int_info, clobbering qede_lock: > > Thanks for highlighting the problem, but actually, the memset is redundant. > We will remove it so the change will not be needed. > > > > > struct qede_dev { > > ... > > struct qed_int_info int_info; > > > > /* Smaller private variant of the RTNL lock */ > > struct mutexqede_lock; > > ... > > > > > > struct qed_int_info { > > struct msix_entry *msix; > > u8 msix_cnt; > > > > /* This should be updated by the protocol driver */ > > u8 used_cnt; > > }; > > > > Should this also clear the "msix" member, or should this not write > > beyond int_info? This patch does the latter. > > It should clear only the msix_cnt, no need to clear the entire > qed_int_info structure. Should used_cnt be cleared too? It is currently. Better yet, what patch do you suggest I replace this proposed one with? :) Thanks for looking at this! -Kees -- Kees Cook
Re: [PATCH 42/64] net: qede: Use memset_after() for counters
On Tue, Jul 27, 2021 at 01:58:33PM -0700, 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_after() 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. > > Signed-off-by: Kees Cook > --- > The old code seems to be doing the wrong thing: starting from not the > first member, but sized for the whole struct. Which is correct? Quick ping on this question. The old code seems to be 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 mutexqede_lock; ... struct qed_int_info { struct msix_entry *msix; u8 msix_cnt; /* This should be updated by the protocol driver */ u8 used_cnt; }; Should this also clear the "msix" member, or should this not write beyond int_info? This patch does the latter. -Kees > --- > 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 01ac1e93d27a..309dfe8c94fb 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(>int_info.msix_cnt, 0, sizeof(struct qed_int_info)); > + memset_after(>int_info, 0, msix); > err3: > qede_napi_disable_remove(edev); > err2: > -- > 2.30.2 > -- Kees Cook
[PATCH 42/64] net: qede: Use memset_after() for counters
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memset(), avoid intentionally writing across neighboring fields. Use memset_after() 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. Signed-off-by: Kees Cook --- The old code seems to be doing the wrong thing: starting from not the first member, but sized for the whole struct. Which is correct? --- 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 01ac1e93d27a..309dfe8c94fb 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(>int_info.msix_cnt, 0, sizeof(struct qed_int_info)); + memset_after(>int_info, 0, msix); err3: qede_napi_disable_remove(edev); err2: -- 2.30.2