From: Colin Ian King <[email protected]>
Currently the for-loop using an unsigned int for the loop counter
which is problematic when comparing it to the signed int count
This is an issue because if the signed int is negative then
the negative loop bound is implicitly cast to an unsigned int on
the comparison to loop counter i and will yield a very large value,
eventually causing an error when memmove/memcpy'ing outside the
allocated region pointed to by ndata.
Fix this by simply making the loop counter i a signed int;
Fixes: f2f2356685bc ("net: dsa: move master ethtool code")
Signed-off-by: Colin Ian King <[email protected]>
---
net/dsa/master.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/dsa/master.c b/net/dsa/master.c
index c58f33931be1..1b659647a303 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -87,8 +87,7 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
struct dsa_switch *ds = cpu_dp->ds;
int port = cpu_dp->index;
int len = ETH_GSTRING_LEN;
- int mcount = 0, count;
- unsigned int i;
+ int mcount = 0, count, i;
uint8_t pfx[4];
uint8_t *ndata;
--
2.20.1
On 3/31/2019 3:12 AM, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> Currently the for-loop using an unsigned int for the loop counter
> which is problematic when comparing it to the signed int count
> This is an issue because if the signed int is negative then
> the negative loop bound is implicitly cast to an unsigned int on
> the comparison to loop counter i and will yield a very large value,
> eventually causing an error when memmove/memcpy'ing outside the
> allocated region pointed to by ndata.
>
> Fix this by simply making the loop counter i a signed int;
>
> Fixes: f2f2356685bc ("net: dsa: move master ethtool code")
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> net/dsa/master.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/dsa/master.c b/net/dsa/master.c
> index c58f33931be1..1b659647a303 100644
> --- a/net/dsa/master.c
> +++ b/net/dsa/master.c
> @@ -87,8 +87,7 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
> struct dsa_switch *ds = cpu_dp->ds;
> int port = cpu_dp->index;
> int len = ETH_GSTRING_LEN;
> - int mcount = 0, count;
> - unsigned int i;
> + int mcount = 0, count, i;
This looks fine but why the return value checking for the negative will
not be good here ?
count = ds->ops->get_sset_count(ds, port, stringset);
Cheers,
Mukesh
> uint8_t pfx[4];
> uint8_t *ndata;
>
On Sun, Mar 31, 2019 at 03:25:47PM +0530, Mukesh Ojha wrote:
>
> On 3/31/2019 3:12 AM, Colin King wrote:
> >From: Colin Ian King <[email protected]>
> >
> >Currently the for-loop using an unsigned int for the loop counter
> >which is problematic when comparing it to the signed int count
> >This is an issue because if the signed int is negative then
> >the negative loop bound is implicitly cast to an unsigned int on
> >the comparison to loop counter i and will yield a very large value,
> >eventually causing an error when memmove/memcpy'ing outside the
> >allocated region pointed to by ndata.
> >
> >Fix this by simply making the loop counter i a signed int;
> >
> >Fixes: f2f2356685bc ("net: dsa: move master ethtool code")
> >Signed-off-by: Colin Ian King <[email protected]>
> >---
> > net/dsa/master.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> >diff --git a/net/dsa/master.c b/net/dsa/master.c
> >index c58f33931be1..1b659647a303 100644
> >--- a/net/dsa/master.c
> >+++ b/net/dsa/master.c
> >@@ -87,8 +87,7 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
> > struct dsa_switch *ds = cpu_dp->ds;
> > int port = cpu_dp->index;
> > int len = ETH_GSTRING_LEN;
> >- int mcount = 0, count;
> >- unsigned int i;
> >+ int mcount = 0, count, i;
>
> This looks fine but why the return value checking for the negative will not
> be good here ?
> ?count = ds->ops->get_sset_count(ds, port, stringset);
Hi Mukesh
Is there a return value check for negative?
Andrew
On 3/31/2019 8:03 PM, Andrew Lunn wrote:
> On Sun, Mar 31, 2019 at 03:25:47PM +0530, Mukesh Ojha wrote:
>> On 3/31/2019 3:12 AM, Colin King wrote:
>>> From: Colin Ian King <[email protected]>
>>>
>>> Currently the for-loop using an unsigned int for the loop counter
>>> which is problematic when comparing it to the signed int count
>>> This is an issue because if the signed int is negative then
>>> the negative loop bound is implicitly cast to an unsigned int on
>>> the comparison to loop counter i and will yield a very large value,
>>> eventually causing an error when memmove/memcpy'ing outside the
>>> allocated region pointed to by ndata.
>>>
>>> Fix this by simply making the loop counter i a signed int;
>>>
>>> Fixes: f2f2356685bc ("net: dsa: move master ethtool code")
>>> Signed-off-by: Colin Ian King <[email protected]>
>>> ---
>>> net/dsa/master.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/net/dsa/master.c b/net/dsa/master.c
>>> index c58f33931be1..1b659647a303 100644
>>> --- a/net/dsa/master.c
>>> +++ b/net/dsa/master.c
>>> @@ -87,8 +87,7 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
>>> struct dsa_switch *ds = cpu_dp->ds;
>>> int port = cpu_dp->index;
>>> int len = ETH_GSTRING_LEN;
>>> - int mcount = 0, count;
>>> - unsigned int i;
>>> + int mcount = 0, count, i;
>> This looks fine but why the return value checking for the negative will not
>> be good here ?
>> count = ds->ops->get_sset_count(ds, port, stringset);
> Hi Mukesh
>
> Is there a return value check for negative?
Just checked looks like dsa_master_get_sset_count can never return -ve
value .
Colin,
do we really get this situation in any scenario ?
Thanks.
Mukesh
Mukesh
>
> Andrew