* [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur [not found] <1380572952-30729-1-git-send-email-andi@firstfloor.org> @ 2013-09-30 20:29 ` Andi Kleen 2013-10-01 15:00 ` Wyborny, Carolyn ` (2 more replies) 2013-09-30 20:29 ` [PATCH 10/11] tcp: Always set options to 0 before calling tcp_established_options Andi Kleen 1 sibling, 3 replies; 6+ messages in thread From: Andi Kleen @ 2013-09-30 20:29 UTC (permalink / raw) To: linux-kernel; +Cc: Andi Kleen, jeffrey.t.kirsher, netdev From: Andi Kleen <ak@linux.intel.com> eee_get_cur assumes that the output data is already zeroed. It can read-modify-write the advertised field: if (ipcnfg & E1000_IPCNFG_EEE_100M_AN) 2594 edata->advertised |= ADVERTISED_100baseT_Full; This is ok for the normal ethtool eee_get call, which always zeroes the input data before. But eee_set_cur also calls eee_get_cur and it did not zero the input field. Later on it then compares agsinst the field, which can contain partial stack garbage. Zero the input field in eee_set_cur() too. Cc: jeffrey.t.kirsher@intel.com Cc: netdev@vger.kernel.org Signed-off-by: Andi Kleen <ak@linux.intel.com> --- drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c index 48cbc83..41e37ff 100644 --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c @@ -2652,6 +2652,8 @@ static int igb_set_eee(struct net_device *netdev, (hw->phy.media_type != e1000_media_type_copper)) return -EOPNOTSUPP; + memset(&eee_curr, 0, sizeof(struct ethtool_eee)); + ret_val = igb_get_eee(netdev, &eee_curr); if (ret_val) return ret_val; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur 2013-09-30 20:29 ` [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur Andi Kleen @ 2013-10-01 15:00 ` Wyborny, Carolyn 2013-10-01 23:10 ` Jeff Kirsher 2013-10-02 20:33 ` David Miller 2 siblings, 0 replies; 6+ messages in thread From: Wyborny, Carolyn @ 2013-10-01 15:00 UTC (permalink / raw) To: Andi Kleen, linux-kernel@vger.kernel.org Cc: Andi Kleen, Kirsher, Jeffrey T, netdev@vger.kernel.org > -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] > On Behalf Of Andi Kleen > Sent: Monday, September 30, 2013 1:29 PM > To: linux-kernel@vger.kernel.org > Cc: Andi Kleen; Kirsher, Jeffrey T; netdev@vger.kernel.org > Subject: [PATCH 07/11] igb: Avoid uninitialized advertised variable in > eee_set_cur > > From: Andi Kleen <ak@linux.intel.com> > > eee_get_cur assumes that the output data is already zeroed. It can read-modify- > write the advertised field: > > if (ipcnfg & E1000_IPCNFG_EEE_100M_AN) > 2594 edata->advertised |= ADVERTISED_100baseT_Full; > > This is ok for the normal ethtool eee_get call, which always zeroes the input > data before. > > But eee_set_cur also calls eee_get_cur and it did not zero the input field. Later > on it then compares agsinst the field, which can contain partial stack garbage. > > Zero the input field in eee_set_cur() too. > > Cc: jeffrey.t.kirsher@intel.com > Cc: netdev@vger.kernel.org > Signed-off-by: Andi Kleen <ak@linux.intel.com> > --- > drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c > b/drivers/net/ethernet/intel/igb/igb_ethtool.c > index 48cbc83..41e37ff 100644 > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c > @@ -2652,6 +2652,8 @@ static int igb_set_eee(struct net_device *netdev, > (hw->phy.media_type != e1000_media_type_copper)) > return -EOPNOTSUPP; > > + memset(&eee_curr, 0, sizeof(struct ethtool_eee)); > + > ret_val = igb_get_eee(netdev, &eee_curr); > if (ret_val) > return ret_val; > -- > 1.8.3.1 > ACK Thanks, Carolyn ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur 2013-09-30 20:29 ` [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur Andi Kleen 2013-10-01 15:00 ` Wyborny, Carolyn @ 2013-10-01 23:10 ` Jeff Kirsher 2013-10-02 20:33 ` David Miller 2 siblings, 0 replies; 6+ messages in thread From: Jeff Kirsher @ 2013-10-01 23:10 UTC (permalink / raw) To: Andi Kleen; +Cc: linux-kernel, Andi Kleen, netdev [-- Attachment #1: Type: text/plain, Size: 949 bytes --] On Mon, 2013-09-30 at 13:29 -0700, Andi Kleen wrote: > From: Andi Kleen <ak@linux.intel.com> > > eee_get_cur assumes that the output data is already zeroed. It can > read-modify-write the advertised field: > > if (ipcnfg & E1000_IPCNFG_EEE_100M_AN) > 2594 edata->advertised |= ADVERTISED_100baseT_Full; > > This is ok for the normal ethtool eee_get call, which always > zeroes the input data before. > > But eee_set_cur also calls eee_get_cur and it did not zero the input > field. Later on it then compares agsinst the field, which can contain > partial > stack garbage. > > Zero the input field in eee_set_cur() too. > > Cc: jeffrey.t.kirsher@intel.com > Cc: netdev@vger.kernel.org > Signed-off-by: Andi Kleen <ak@linux.intel.com> > --- > drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++ > 1 file changed, 2 insertions(+) Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur 2013-09-30 20:29 ` [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur Andi Kleen 2013-10-01 15:00 ` Wyborny, Carolyn 2013-10-01 23:10 ` Jeff Kirsher @ 2013-10-02 20:33 ` David Miller 2 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2013-10-02 20:33 UTC (permalink / raw) To: andi; +Cc: linux-kernel, ak, jeffrey.t.kirsher, netdev From: Andi Kleen <andi@firstfloor.org> Date: Mon, 30 Sep 2013 13:29:08 -0700 > From: Andi Kleen <ak@linux.intel.com> > > eee_get_cur assumes that the output data is already zeroed. It can > read-modify-write the advertised field: > > if (ipcnfg & E1000_IPCNFG_EEE_100M_AN) > 2594 edata->advertised |= ADVERTISED_100baseT_Full; > > This is ok for the normal ethtool eee_get call, which always > zeroes the input data before. > > But eee_set_cur also calls eee_get_cur and it did not zero the input > field. Later on it then compares agsinst the field, which can contain partial > stack garbage. > > Zero the input field in eee_set_cur() too. > > Cc: jeffrey.t.kirsher@intel.com > Cc: netdev@vger.kernel.org > Signed-off-by: Andi Kleen <ak@linux.intel.com> Applied. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 10/11] tcp: Always set options to 0 before calling tcp_established_options [not found] <1380572952-30729-1-git-send-email-andi@firstfloor.org> 2013-09-30 20:29 ` [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur Andi Kleen @ 2013-09-30 20:29 ` Andi Kleen 2013-10-02 20:33 ` David Miller 1 sibling, 1 reply; 6+ messages in thread From: Andi Kleen @ 2013-09-30 20:29 UTC (permalink / raw) To: linux-kernel; +Cc: Andi Kleen, netdev From: Andi Kleen <ak@linux.intel.com> tcp_established_options assumes opts->options is 0 before calling, as it read modify writes it. For the tcp_current_mss() case the opts structure is not zeroed, so this can be done with uninitialized values. This is ok, because ->options is not read in this path. But it's still better to avoid the operation on the uninitialized field. This shuts up a static code analyzer, and presumably may help the optimizer. Cc: netdev@vger.kernel.org Signed-off-by: Andi Kleen <ak@linux.intel.com> --- net/ipv4/tcp_output.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 7c83cb8..f3ed78d 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -637,6 +637,8 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb unsigned int size = 0; unsigned int eff_sacks; + opts->options = 0; + #ifdef CONFIG_TCP_MD5SIG *md5 = tp->af_specific->md5_lookup(sk, sk); if (unlikely(*md5)) { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 10/11] tcp: Always set options to 0 before calling tcp_established_options 2013-09-30 20:29 ` [PATCH 10/11] tcp: Always set options to 0 before calling tcp_established_options Andi Kleen @ 2013-10-02 20:33 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2013-10-02 20:33 UTC (permalink / raw) To: andi; +Cc: linux-kernel, ak, netdev From: Andi Kleen <andi@firstfloor.org> Date: Mon, 30 Sep 2013 13:29:11 -0700 > From: Andi Kleen <ak@linux.intel.com> > > tcp_established_options assumes opts->options is 0 before calling, > as it read modify writes it. > > For the tcp_current_mss() case the opts structure is not zeroed, > so this can be done with uninitialized values. > > This is ok, because ->options is not read in this path. > But it's still better to avoid the operation on the uninitialized > field. This shuts up a static code analyzer, and presumably > may help the optimizer. > > Cc: netdev@vger.kernel.org > Signed-off-by: Andi Kleen <ak@linux.intel.com> Applied. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-02 20:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1380572952-30729-1-git-send-email-andi@firstfloor.org>
2013-09-30 20:29 ` [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur Andi Kleen
2013-10-01 15:00 ` Wyborny, Carolyn
2013-10-01 23:10 ` Jeff Kirsher
2013-10-02 20:33 ` David Miller
2013-09-30 20:29 ` [PATCH 10/11] tcp: Always set options to 0 before calling tcp_established_options Andi Kleen
2013-10-02 20:33 ` David Miller
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).