Message ID | 20190330214241.15311-1-colin.king@canonical.com |
---|---|
State | Deferred |
Delegated to: | David Miller |
Headers | show |
Series | net: dsa: fix negative loop bound error on for loop | expand |
On 3/31/2019 3:12 AM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > 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 <colin.king@canonical.com> > --- > 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 <colin.king@canonical.com> > > > >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 <colin.king@canonical.com> > >--- > > 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 <colin.king@canonical.com> >>> >>> 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 <colin.king@canonical.com> >>> --- >>> 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
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;