[patch] pch_gbe: memory corruption calling pch_gbe_validate_option()

[patch] pch_gbe: memory corruption calling pch_gbe_validate_option()

[Dan Carpenter]

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>

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);

Re: [patch] pch_gbe: memory corruption calling pch_gbe_validate_option()

[David Miller]

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.

Re: [patch] pch_gbe: memory corruption calling pch_gbe_validate_option()

[santosh prasad nayak]

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

Re: [patch] pch_gbe: memory corruption calling pch_gbe_validate_option()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]