Message ID | 20120301071708.GB6959@elgon.mountain |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Dan Carpenter <dan.carpenter@oracle.com> Date: Thu, 1 Mar 2012 10:17:08 +0300 > pch_gbe_validate_option() modifies 32 bits of memory but we pass > &hw->phy.autoneg_advertised which only has 16 bits and &hw->mac.fc > which only has 8 bits. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dan, Your fix may introduce new bug. hw->phy.autoneg_advertised = tmp Assigning signed integer to unsigned short leads to bit truncation. Is it safe for both Big-endian and little endian format ? AutoNeg is initialized with a Negative value (OPTION_UNSET) Won't it create any issue with above assignment ? The simpler fix is to make "autoneg_advertised" signed integer. struct pch_gbe_phy_info { u32 addr; u32 id; u32 revision; u32 reset_delay_us; u16 autoneg_advertised; // ==> int autoneg_advertised }; Regards Santosh On Fri, Mar 2, 2012 at 3:54 AM, David Miller <davem@davemloft.net> wrote: > From: Dan Carpenter <dan.carpenter@oracle.com> > Date: Thu, 1 Mar 2012 10:17:08 +0300 > >> pch_gbe_validate_option() modifies 32 bits of memory but we pass >> &hw->phy.autoneg_advertised which only has 16 bits and &hw->mac.fc >> which only has 8 bits. >> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Applied. > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 05, 2012 at 11:04:49AM +0530, santosh prasad nayak wrote: > Dan, > > Your fix may introduce new bug. > > hw->phy.autoneg_advertised = tmp > > Assigning signed integer to unsigned short leads to bit truncation. In this case it doesn't. It's going to be 0x2f or less. I obviously checked this before I submitted the patch. > Is it safe for both Big-endian and little endian format ? It's CPU endian in both cases. > > AutoNeg is initialized with a Negative value (OPTION_UNSET) > Won't it create any issue with above assignment ? > Nope. It gets set to 0x2f inside pch_gbe_validate_option(). > > The simpler fix is to make "autoneg_advertised" signed integer. > > struct pch_gbe_phy_info { > u32 addr; > u32 id; > u32 revision; > u32 reset_delay_us; > u16 autoneg_advertised; // ==> int autoneg_advertised > }; > The better fix would be to change pch_gbe_validate_option() so it isn't so easy to call improperly. I would have done that except that probably we won't introduce many more callers, it seemed like a lot of work and I don't have the hardware to test the results. regards, dan carpenter
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_param.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_param.c index 9cb5f91..29e23be 100644 --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_param.c +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_param.c @@ -321,10 +321,10 @@ static void pch_gbe_check_copper_options(struct pch_gbe_adapter *adapter) pr_debug("AutoNeg specified along with Speed or Duplex, AutoNeg parameter ignored\n"); hw->phy.autoneg_advertised = opt.def; } else { - hw->phy.autoneg_advertised = AutoNeg; - pch_gbe_validate_option( - (int *)(&hw->phy.autoneg_advertised), - &opt, adapter); + int tmp = AutoNeg; + + pch_gbe_validate_option(&tmp, &opt, adapter); + hw->phy.autoneg_advertised = tmp; } } @@ -495,9 +495,10 @@ void pch_gbe_check_options(struct pch_gbe_adapter *adapter) .arg = { .l = { .nr = (int)ARRAY_SIZE(fc_list), .p = fc_list } } }; - hw->mac.fc = FlowControl; - pch_gbe_validate_option((int *)(&hw->mac.fc), - &opt, adapter); + int tmp = FlowControl; + + pch_gbe_validate_option(&tmp, &opt, adapter); + hw->mac.fc = tmp; } pch_gbe_check_copper_options(adapter);
pch_gbe_validate_option() modifies 32 bits of memory but we pass &hw->phy.autoneg_advertised which only has 16 bits and &hw->mac.fc which only has 8 bits. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html