diff mbox

[Suggestion] net/atm : for sprintf, need check the total write length whether larger than a page.

Message ID 201212040346.qB43kVSQ018028@thirdoffive.cmf.nrl.navy.mil
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

chas williams - CONTRACTOR Dec. 4, 2012, 3:46 a.m. UTC 
In message <50AC58BC.1020004@asianux.com>,Chen Gang writes:
>in net/atm/atm_sysfs.c:
>  suggest to check the write length whether larger than a page.
>  the length of parameter buf is one page size (reference: fill_read_buffer at fs/sysfs/file.c)
>  and the count of atm adresses are not limited (reference: atm_dev_ioctl -> atm_add_addr)
>
>  thanks.
>
>gchen.

how about this as a possible fix?

atm: use scnprintf() instead of sprintf()

As reported by Chen Gang <gang.chen@asianux.com>, we should ensure there
is enough space when formatting the sysfs buffers.

Signed-off-by: Chas Williams <chas@cmf.nrl.navy.mil>
---
 net/atm/atm_sysfs.c |   40 +++++++++++++++-------------------------
 1 files changed, 15 insertions(+), 25 deletions(-)

Comments

Chen Gang Dec. 5, 2012, 1:26 a.m. UTC | #1 
firstly, sorry for reply late (yesterday I have an annual leave for
some personal things).

  and thank you for your reply, too.


于 2012年12月04日 11:46, Chas Williams (CONTRACTOR) 写道:
>  static ssize_t show_address(struct device *cdev,
>  			    struct device_attribute *attr, char *buf)
>  {
> -	char *pos = buf;
>  	struct atm_dev *adev = to_atm_dev(cdev);
> -	int i;
> -
> -	for (i = 0; i < (ESI_LEN - 1); i++)
> -		pos += sprintf(pos, "%02x:", adev->esi[i]);
> -	pos += sprintf(pos, "%02x\n", adev->esi[i]);
>  
> -	return pos - buf;
> +	return scnprintf(buf, PAGE_SIZE, "%pM\n", adev->esi);
>  }
>  

  "%p" seems print a pointer, not contents of pointer (is it correct ?)

  will it change the original display format to outside ?


>  static ssize_t show_atmaddress(struct device *cdev,
>  			       struct device_attribute *attr, char *buf)
>  {
>  	unsigned long flags;
> -	char *pos = buf;
>  	struct atm_dev *adev = to_atm_dev(cdev);
>  	struct atm_dev_addr *aaddr;
>  	int bin[] = { 1, 2, 10, 6, 1 }, *fmt = bin;
> -	int i, j;
> +	int i, j, count = 0;
>  
>  	spin_lock_irqsave(&adev->lock, flags);
>  	list_for_each_entry(aaddr, &adev->local, entry) {
>  		for (i = 0, j = 0; i < ATM_ESA_LEN; ++i, ++j) {
>  			if (j == *fmt) {
> -				pos += sprintf(pos, ".");
> +				count += scnprintf(buf + count,
> +						   PAGE_SIZE - count, ".");
>  				++fmt;
>  				j = 0;
>  			}
> -			pos += sprintf(pos, "%02x",
> -				       aaddr->addr.sas_addr.prv[i]);
> +			count += scnprintf(buf + count,
> +					   PAGE_SIZE - count, "%02x",
> +					   aaddr->addr.sas_addr.prv[i]);
>  		}
> -		pos += sprintf(pos, "\n");
> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
>  	}
>  	spin_unlock_irqrestore(&adev->lock, flags);
>  
> -	return pos - buf;
> +	return count;
>  }
>  

  need we judge whether count >= PAGE_SIZE ?


  these are my suggestions, maybe not correct.

  :-)

  thanks.
chas williams - CONTRACTOR Dec. 5, 2012, 3:57 a.m. UTC | #2 
In message <50BEA2CB.9000800@asianux.com>,Chen Gang writes:
>> -	for (i = 0; i < (ESI_LEN - 1); i++)
>> -		pos += sprintf(pos, "%02x:", adev->esi[i]);
>> -	pos += sprintf(pos, "%02x\n", adev->esi[i]);
>>  
>> -	return pos - buf;
>> +	return scnprintf(buf, PAGE_SIZE, "%pM\n", adev->esi);
>>  }
>>  
>
>  "%p" seems print a pointer, not contents of pointer (is it correct ?)
>  will it change the original display format to outside ?

%pM means format this pointer as a mac address.  it didnt exist when the
atm stack was originally written but can be used now to save a bit of
messy code.

>> -		pos += sprintf(pos, "\n");
>> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
...
>  need we judge whether count >= PAGE_SIZE ?

count will eventually make PAGE_SIZE - count reach 0 at which point,
scnprintf() won't be able to write into the buffer.
--
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
Chen Gang Dec. 5, 2012, 4:56 a.m. UTC | #3 
于 2012年12月05日 11:57, Chas Williams (CONTRACTOR) 写道:
> In message <50BEA2CB.9000800@asianux.com>,Chen Gang writes:
>>> -	for (i = 0; i < (ESI_LEN - 1); i++)
>>> -		pos += sprintf(pos, "%02x:", adev->esi[i]);
>>> -	pos += sprintf(pos, "%02x\n", adev->esi[i]);
>>>  
>>> -	return pos - buf;
>>> +	return scnprintf(buf, PAGE_SIZE, "%pM\n", adev->esi);
>>>  }
>>>  
>>
>>  "%p" seems print a pointer, not contents of pointer (is it correct ?)
>>  will it change the original display format to outside ?
> 
> %pM means format this pointer as a mac address.  it didnt exist when the
> atm stack was originally written but can be used now to save a bit of
> messy code.
> 

  it is my fault. thank you

  :-)


>>> -		pos += sprintf(pos, "\n");
>>> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
> ..
>>  need we judge whether count >= PAGE_SIZE ?
> 
> count will eventually make PAGE_SIZE - count reach 0 at which point,
> scnprintf() won't be able to write into the buffer.

  I also think so.

  I think, maybe it will be better to break the loop when we already
know that "count >= PAGE_SIZE" (it can save waste looping, although it
seems unlikly happen, for example, using unlikly(...) ).

By the way:
  will it be better that always let "\n" at the end ?
  (if count == PAGE_SIZE in a loop, we can not let "\n" at the end).



  I think what I said above are minor, if you think, for this patch, do
not need consider them, it is ok (at least for me, it is true).

  :-)

> --
> 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
> 
>
Chen Gang Dec. 5, 2012, 5:40 a.m. UTC | #4 
于 2012年12月05日 12:56, Chen Gang 写道:
>>>> >>> -		pos += sprintf(pos, "\n");
>>>> >>> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
>> > ..
>>> >>  need we judge whether count >= PAGE_SIZE ?
>> > 
>> > count will eventually make PAGE_SIZE - count reach 0 at which point,
>> > scnprintf() won't be able to write into the buffer.
>   I also think so.
> 
>   I think, maybe it will be better to break the loop when we already
> know that "count >= PAGE_SIZE" (it can save waste looping, although it
> seems unlikly happen, for example, using unlikly(...) ).
> 
> By the way:
>   will it be better that always let "\n" at the end ?
>   (if count == PAGE_SIZE in a loop, we can not let "\n" at the end).

   oh, sorry ! count will never >= PAGE_SIZE.

   I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we
can make the room for the end of "\n".
Chen Gang Dec. 5, 2012, 5:59 a.m. UTC | #5 
于 2012年12月05日 13:40, Chen Gang 写道:
> 于 2012年12月05日 12:56, Chen Gang 写道:
>>>>>>>> -		pos += sprintf(pos, "\n");
>>>>>>>> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
>>>> ..
>>>>>>  need we judge whether count >= PAGE_SIZE ?
>>>>
>>>> count will eventually make PAGE_SIZE - count reach 0 at which point,
>>>> scnprintf() won't be able to write into the buffer.
>>   I also think so.
>>
>>   I think, maybe it will be better to break the loop when we already
>> know that "count >= PAGE_SIZE" (it can save waste looping, although it
>> seems unlikly happen, for example, using unlikly(...) ).
>>
>> By the way:
>>   will it be better that always let "\n" at the end ?
>>   (if count == PAGE_SIZE in a loop, we can not let "\n" at the end).
> 
>    oh, sorry ! count will never >= PAGE_SIZE.
> 
>    I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we
> can make the room for the end of "\n".
> 
> 
> 
   sorry, "PAGE_SIZE - 1" is enough, not need "PAGE_SIZE - 2".
chas williams - CONTRACTOR Dec. 5, 2012, 2:55 p.m. UTC | #6 
On Wed, 05 Dec 2012 13:59:26 +0800
Chen Gang <gang.chen@asianux.com> wrote:

> 于 2012年12月05日 13:40, Chen Gang 写道:
> > 于 2012年12月05日 12:56, Chen Gang 写道:
> >>>>>>>> -		pos += sprintf(pos, "\n");
> >>>>>>>> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
> >>>> ..
> >>>>>>  need we judge whether count >= PAGE_SIZE ?
> >>>>
> >>>> count will eventually make PAGE_SIZE - count reach 0 at which point,
> >>>> scnprintf() won't be able to write into the buffer.
> >>   I also think so.
> >>
> >>   I think, maybe it will be better to break the loop when we already
> >> know that "count >= PAGE_SIZE" (it can save waste looping, although it
> >> seems unlikly happen, for example, using unlikly(...) ).

it doesn't seem like optimizing for this corner case is a huge
concern.  the list cannot be infinitely long.

> >>
> >> By the way:
> >>   will it be better that always let "\n" at the end ?
> >>   (if count == PAGE_SIZE in a loop, we can not let "\n" at the end).
> > 
> >    oh, sorry ! count will never >= PAGE_SIZE.
> > 
> >    I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we
> > can make the room for the end of "\n".
> > 
> > 
> > 
>    sorry, "PAGE_SIZE - 1" is enough, not need "PAGE_SIZE - 2".

did you mean '\0' instead of '\n'?  scnprintf() considers the trailing
'\0' when formatting.
--
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
Chen Gang Dec. 6, 2012, 1:15 a.m. UTC | #7 
于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道:
> On Wed, 05 Dec 2012 13:59:26 +0800
> 
> it doesn't seem like optimizing for this corner case is a huge
> concern.  the list cannot be infinitely long.
> 

  ok.


>>>>
>>>> By the way:
>>>>   will it be better that always let "\n" at the end ?
>>>>   (if count == PAGE_SIZE in a loop, we can not let "\n" at the end).
>>>
>>>    oh, sorry ! count will never >= PAGE_SIZE.
>>>
>>>    I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we
>>> can make the room for the end of "\n".
>>>
>>>
>>>
>>    sorry, "PAGE_SIZE - 1" is enough, not need "PAGE_SIZE - 2".
> 
> did you mean '\0' instead of '\n'?  scnprintf() considers the trailing
> '\0' when formatting.

  no, originally, the end is "\n\0".

  I prefer we still compatible "\n" when the contents are very large.
  if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end.

-		pos += sprintf(pos, "\n");
+		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");


> --
> 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
> 
>
Chen Gang Dec. 6, 2012, 9:05 a.m. UTC | #8 
Hi Chas Williams:

  all of my original reply are my idea (or suggestions), not for issues.

  if you do not need them, please help to send patch.

  I have tried to check it. at least, I did not find issues (and I also
also learned from it)

  hope the patch can pass reviewers checking !

  Good Luck !

  :-)

gchen.

于 2012年12月06日 09:15, Chen Gang 写道:
> 于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道:
>> On Wed, 05 Dec 2012 13:59:26 +0800
>>
>> it doesn't seem like optimizing for this corner case is a huge
>> concern.  the list cannot be infinitely long.
>>
> 
>   ok.
> 
> 
>>>>>
>>>>> By the way:
>>>>>   will it be better that always let "\n" at the end ?
>>>>>   (if count == PAGE_SIZE in a loop, we can not let "\n" at the end).
>>>>
>>>>    oh, sorry ! count will never >= PAGE_SIZE.
>>>>
>>>>    I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we
>>>> can make the room for the end of "\n".
>>>>
>>>>
>>>>
>>>    sorry, "PAGE_SIZE - 1" is enough, not need "PAGE_SIZE - 2".
>>
>> did you mean '\0' instead of '\n'?  scnprintf() considers the trailing
>> '\0' when formatting.
> 
>   no, originally, the end is "\n\0".
> 
>   I prefer we still compatible "\n" when the contents are very large.
>   if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end.
> 
> -		pos += sprintf(pos, "\n");
> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
> 
> 
>> --
>> 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
>>
>>
> 
>
chas williams - CONTRACTOR Dec. 6, 2012, 2:08 p.m. UTC | #9 
On Thu, 06 Dec 2012 09:15:10 +0800
Chen Gang <gang.chen@asianux.com> wrote:

> 于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道:

> > did you mean '\0' instead of '\n'?  scnprintf() considers the trailing
> > '\0' when formatting.
> 
>   no, originally, the end is "\n\0".
> 
>   I prefer we still compatible "\n" when the contents are very large.
>   if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end.
> 
> -		pos += sprintf(pos, "\n");
> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");

i would make the code a bit messy to do this for not much gain.  again,
it isnt likely you would run into this in a normal situation.
--
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
Chen Gang Dec. 7, 2012, 1:07 a.m. UTC | #10 
于 2012年12月06日 22:08, chas williams - CONTRACTOR 写道:
> On Thu, 06 Dec 2012 09:15:10 +0800
> Chen Gang <gang.chen@asianux.com> wrote:
> 
>> 于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道:
> 
>>> did you mean '\0' instead of '\n'?  scnprintf() considers the trailing
>>> '\0' when formatting.
>>
>>   no, originally, the end is "\n\0".
>>
>>   I prefer we still compatible "\n" when the contents are very large.
>>   if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end.
>>
>> -		pos += sprintf(pos, "\n");
>> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
> 
> i would make the code a bit messy to do this for not much gain.  again,
> it isnt likely you would run into this in a normal situation.

  surely.

  thanks.

> --
> 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
> 
>
Chen Gang Dec. 10, 2012, 1:39 a.m. UTC | #11 
Hello Chas Williams:

  Excuse me:
    I am a reporter (not a reviewer), and not suitable to review another's patch.

  so:
    I suggest you to send your patch (as your willing) to the reviewers, directly.
    and need not cc to me (I am not a reviewer).

  thanks.

gchen.


于 2012年12月07日 09:07, Chen Gang 写道:
> 于 2012年12月06日 22:08, chas williams - CONTRACTOR 写道:
>> On Thu, 06 Dec 2012 09:15:10 +0800
>> Chen Gang <gang.chen@asianux.com> wrote:
>>
>>> 于 2012年12月05日 22:55, chas williams - CONTRACTOR 写道:
>>
>>>> did you mean '\0' instead of '\n'?  scnprintf() considers the trailing
>>>> '\0' when formatting.
>>>
>>>   no, originally, the end is "\n\0".
>>>
>>>   I prefer we still compatible "\n" when the contents are very large.
>>>   if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end.
>>>
>>> -		pos += sprintf(pos, "\n");
>>> +		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
>>
>> i would make the code a bit messy to do this for not much gain.  again,
>> it isnt likely you would run into this in a normal situation.
> 
>   surely.
> 
>   thanks.
> 
>> --
>> 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
>>
>>
> 
>
diff mbox

Patch

diff --git a/net/atm/atm_sysfs.c b/net/atm/atm_sysfs.c
index f49da58..350bf62 100644
--- a/net/atm/atm_sysfs.c
+++ b/net/atm/atm_sysfs.c
@@ -14,49 +14,45 @@  static ssize_t show_type(struct device *cdev,
 			 struct device_attribute *attr, char *buf)
 {
 	struct atm_dev *adev = to_atm_dev(cdev);
-	return sprintf(buf, "%s\n", adev->type);
+
+	return scnprintf(buf, PAGE_SIZE, "%s\n", adev->type);
 }
 
 static ssize_t show_address(struct device *cdev,
 			    struct device_attribute *attr, char *buf)
 {
-	char *pos = buf;
 	struct atm_dev *adev = to_atm_dev(cdev);
-	int i;
-
-	for (i = 0; i < (ESI_LEN - 1); i++)
-		pos += sprintf(pos, "%02x:", adev->esi[i]);
-	pos += sprintf(pos, "%02x\n", adev->esi[i]);
 
-	return pos - buf;
+	return scnprintf(buf, PAGE_SIZE, "%pM\n", adev->esi);
 }
 
 static ssize_t show_atmaddress(struct device *cdev,
 			       struct device_attribute *attr, char *buf)
 {
 	unsigned long flags;
-	char *pos = buf;
 	struct atm_dev *adev = to_atm_dev(cdev);
 	struct atm_dev_addr *aaddr;
 	int bin[] = { 1, 2, 10, 6, 1 }, *fmt = bin;
-	int i, j;
+	int i, j, count = 0;
 
 	spin_lock_irqsave(&adev->lock, flags);
 	list_for_each_entry(aaddr, &adev->local, entry) {
 		for (i = 0, j = 0; i < ATM_ESA_LEN; ++i, ++j) {
 			if (j == *fmt) {
-				pos += sprintf(pos, ".");
+				count += scnprintf(buf + count,
+						   PAGE_SIZE - count, ".");
 				++fmt;
 				j = 0;
 			}
-			pos += sprintf(pos, "%02x",
-				       aaddr->addr.sas_addr.prv[i]);
+			count += scnprintf(buf + count,
+					   PAGE_SIZE - count, "%02x",
+					   aaddr->addr.sas_addr.prv[i]);
 		}
-		pos += sprintf(pos, "\n");
+		count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
 	}
 	spin_unlock_irqrestore(&adev->lock, flags);
 
-	return pos - buf;
+	return count;
 }
 
 static ssize_t show_atmindex(struct device *cdev,
@@ -64,25 +60,21 @@  static ssize_t show_atmindex(struct device *cdev,
 {
 	struct atm_dev *adev = to_atm_dev(cdev);
 
-	return sprintf(buf, "%d\n", adev->number);
+	return scnprintf(buf, PAGE_SIZE, "%d\n", adev->number);
 }
 
 static ssize_t show_carrier(struct device *cdev,
 			    struct device_attribute *attr, char *buf)
 {
-	char *pos = buf;
 	struct atm_dev *adev = to_atm_dev(cdev);
 
-	pos += sprintf(pos, "%d\n",
-		       adev->signal == ATM_PHY_SIG_LOST ? 0 : 1);
-
-	return pos - buf;
+	return scnprintf(buf, PAGE_SIZE, "%d\n",
+			 adev->signal == ATM_PHY_SIG_LOST ? 0 : 1);
 }
 
 static ssize_t show_link_rate(struct device *cdev,
 			      struct device_attribute *attr, char *buf)
 {
-	char *pos = buf;
 	struct atm_dev *adev = to_atm_dev(cdev);
 	int link_rate;
 
@@ -100,9 +92,7 @@  static ssize_t show_link_rate(struct device *cdev,
 	default:
 		link_rate = adev->link_rate * 8 * 53;
 	}
-	pos += sprintf(pos, "%d\n", link_rate);
-
-	return pos - buf;
+	return scnprintf(buf, PAGE_SIZE, "%d\n", link_rate);
 }
 
 static DEVICE_ATTR(address, S_IRUGO, show_address, NULL);