| 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