[PATCH net-next] bonding: fix strlen errors in sysfs

[PATCH net-next] bonding: fix strlen errors in sysfs

[Andy Gospodarek]

When a bond contains a device where one name is the subset of another
(eth1 and eth10, for example), one cannot properly set the primary
device or the currently active device.

This was reported and based on work by Takuma Umeya.  I also verified
the problem and tested that this fix resolves it.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
Reported-by: Takuma Umeya <tumeya@redhat.com>
---
 drivers/net/bonding/bond_sysfs.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index b60835f..50e859c 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1037,9 +1037,9 @@ static ssize_t bonding_store_primary(struct device *d,
 			bond->dev->name, bond->dev->name, bond->params.mode);
 	} else {
 		bond_for_each_slave(bond, slave, i) {
-			if (strnicmp
-			    (slave->dev->name, buf,
-			     strlen(slave->dev->name)) == 0) {
+			int max_len = max(strlen(slave->dev->name),
+					  strlen(buf) - 1);
+			if (strnicmp(slave->dev->name, buf, max_len) == 0) {
 				pr_info("%s: Setting %s as primary slave.\n",
 					bond->dev->name, slave->dev->name);
 				bond->primary_slave = slave;
@@ -1208,9 +1208,9 @@ static ssize_t bonding_store_active_slave(struct device *d,
 			bond->dev->name, bond->dev->name, bond->params.mode);
 	else {
 		bond_for_each_slave(bond, slave, i) {
-			if (strnicmp
-			    (slave->dev->name, buf,
-			     strlen(slave->dev->name)) == 0) {
+			int max_len = max(strlen(slave->dev->name),
+					  strlen(buf) - 1);
+			if (strnicmp(slave->dev->name, buf, max_len) == 0) {
         			old_active = bond->curr_active_slave;
         			new_active = slave;
         			if (new_active == old_active) {
-- 
1.7.4.4

Re: [PATCH net-next] bonding: fix strlen errors in sysfs

[Vitalii Demianets]

On Thursday 14 July 2011 04:57:45 Andy Gospodarek wrote:
> -			if (strnicmp
> -			    (slave->dev->name, buf,
> -			     strlen(slave->dev->name)) == 0) {
> +			int max_len = max(strlen(slave->dev->name),
> +					  strlen(buf) - 1);
> +			if (strnicmp(slave->dev->name, buf, max_len) == 0) {

As for me there is no sense in preventing "address out of range" errors in  
strnicmp by calculating length with strlen first. If there is missing \0 at 
the end of the string you just shift failure point from stricmp to the strlen 
function call.
IMHO "maximum length" argument in strnicmp should be some appropriate constant 
instead. Alternatively we can use count:
 if (strnicmp(slave->dev->name, buf, count) == 0) {

> -			if (strnicmp
> -			    (slave->dev->name, buf,
> -			     strlen(slave->dev->name)) == 0) {
> +			int max_len = max(strlen(slave->dev->name),
> +					  strlen(buf) - 1);
> +			if (strnicmp(slave->dev->name, buf, max_len) == 0) {

Same here.

-- 
Vitalii

Re: [PATCH net-next] bonding: fix strlen errors in sysfs

[Jay Vosburgh]

Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote:

>On Thursday 14 July 2011 04:57:45 Andy Gospodarek wrote:
>> -			if (strnicmp
>> -			    (slave->dev->name, buf,
>> -			     strlen(slave->dev->name)) == 0) {
>> +			int max_len = max(strlen(slave->dev->name),
>> +					  strlen(buf) - 1);
>> +			if (strnicmp(slave->dev->name, buf, max_len) == 0) {
>
>As for me there is no sense in preventing "address out of range" errors in  
>strnicmp by calculating length with strlen first. If there is missing \0 at 
>the end of the string you just shift failure point from stricmp to the strlen 
>function call.
>IMHO "maximum length" argument in strnicmp should be some appropriate constant 
>instead. Alternatively we can use count:

	I agree about using a constant, and I nominate IFNAMSIZ for that
constant.

	Also, should we really be using strnicmp?  I.e., case
insensitive?  Aren't interface names case sensitive?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next v2] bonding: fix strlen errors in sysfs

[Andy Gospodarek]

On Thu, Jul 14, 2011 at 09:02:06AM -0700, Jay Vosburgh wrote:
> Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote:
> 
> >On Thursday 14 July 2011 04:57:45 Andy Gospodarek wrote:
> >> -			if (strnicmp
> >> -			    (slave->dev->name, buf,
> >> -			     strlen(slave->dev->name)) == 0) {
> >> +			int max_len = max(strlen(slave->dev->name),
> >> +					  strlen(buf) - 1);
> >> +			if (strnicmp(slave->dev->name, buf, max_len) == 0) {
> >
> >As for me there is no sense in preventing "address out of range" errors in  
> >strnicmp by calculating length with strlen first. If there is missing \0 at 
> >the end of the string you just shift failure point from stricmp to the strlen 
> >function call.
> >IMHO "maximum length" argument in strnicmp should be some appropriate constant 
> >instead. Alternatively we can use count:
> 
> 	I agree about using a constant, and I nominate IFNAMSIZ for that
> constant.
> 

A constant like IFNAMSIZ can work, but only if buf has the '\n' removed
from the string that is added by echo or other command first.

> 	Also, should we really be using strnicmp?  I.e., case
> insensitive?  Aren't interface names case sensitive?

Probably not.

I'll roll a patch next week that drops the newline and uses IFNAMSIZ if
that is the preference.  I didn't think it was worth the trouble
initially, so I didn't do it that way the first time.