Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1598461 > unrolled thread

[PATCH] staging: android: Replace strcpy with strlcpy

Started bysimran singhal <singhalsimran0@gmail.com>
First post2017-03-11 21:50 +0100
Last post2017-03-13 13:50 +0100
Articles 6 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging: android: Replace strcpy with strlcpy simran singhal <singhalsimran0@gmail.com> - 2017-03-11 21:50 +0100
    Re: [PATCH] staging: android: Replace strcpy with strlcpy Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-12 02:10 +0100
    Re: [PATCH] staging: android: Replace strcpy with strlcpy SIMRAN SINGHAL <singhalsimran0@gmail.com> - 2017-03-13 13:50 +0100
      Re: [PATCH] staging: android: Replace strcpy with strlcpy Dan Carpenter <dan.carpenter@oracle.com> - 2017-03-13 14:00 +0100
        Re: [PATCH] staging: android: Replace strcpy with strlcpy SIMRAN SINGHAL <singhalsimran0@gmail.com> - 2017-03-13 14:20 +0100
    Re: [PATCH] staging: android: Replace strcpy with strlcpy Dan Carpenter <dan.carpenter@oracle.com> - 2017-03-13 13:50 +0100

#1598461 — [PATCH] staging: android: Replace strcpy with strlcpy

Fromsimran singhal <singhalsimran0@gmail.com>
Date2017-03-11 21:50 +0100
Subject[PATCH] staging: android: Replace strcpy with strlcpy
Message-ID<tjW7D-827-5@gated-at.bofh.it>
Replace strcpy with strlcpy as strcpy does not check for buffer
overflow.
This is found using Flawfinder.

Signed-off-by: simran singhal <singhalsimran0@gmail.com>
---
 drivers/staging/android/ashmem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 7cbad0d..eb2f4ef 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
 	if (unlikely(asma->file))
 		ret = -EINVAL;
 	else
-		strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
+		strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
+			sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
 
 	mutex_unlock(&ashmem_mutex);
 	return ret;
-- 
2.7.4

[toc] | [next] | [standalone]

#1598518

FromAl Viro <viro@ZenIV.linux.org.uk>
Date2017-03-12 02:10 +0100
Message-ID<tk0bf-2A1-3@gated-at.bofh.it>
In reply to#1598461
On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
> Replace strcpy with strlcpy as strcpy does not check for buffer
> overflow.
> This is found using Flawfinder.
> 
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> ---
>  drivers/staging/android/ashmem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index 7cbad0d..eb2f4ef 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>  	if (unlikely(asma->file))
>  		ret = -EINVAL;
>  	else
> -		strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
> +		strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
> +			sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));

Trivial C quiz: given

struct ashmem_area {
        char name[ASHMEM_FULL_NAME_LEN];
        struct list_head unpinned_list;
        struct file *file;
        size_t size;
        unsigned long prot_mask;
};
static int set_name(struct ashmem_area *asma, void __user *name)

what, in your opinion, would be
	1) type of asma->name
	2) type of asma->name + ASHMEM_NAME_PREFIX_LEN
	3) value of sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN)
As a bonus question,
	4) what is the value of this kind of patches?

<rot13 answers>
	1) NFUZRZ_SHYY_ANZR_YRA-ryrzrag neenl bs pune
	2) cbvagre gb pune
	3) fvmr bs n cbvagre
	4) fbpvbybtvpny - ernql-znqr vyyhfgengvbaf bs crevyf bs
pnetb phyg.

[toc] | [prev] | [next] | [standalone]

#1599295

FromSIMRAN SINGHAL <singhalsimran0@gmail.com>
Date2017-03-13 13:50 +0100
Message-ID<tkxAd-ys-1@gated-at.bofh.it>
In reply to#1598461
On Mon, Mar 13, 2017 at 6:11 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
>> Replace strcpy with strlcpy as strcpy does not check for buffer
>> overflow.
>> This is found using Flawfinder.
>>
>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>> ---
>>  drivers/staging/android/ashmem.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
>> index 7cbad0d..eb2f4ef 100644
>> --- a/drivers/staging/android/ashmem.c
>> +++ b/drivers/staging/android/ashmem.c
>> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>>       if (unlikely(asma->file))
>>               ret = -EINVAL;
>>       else
>> -             strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
>> +             strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
>> +                     sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This isn't right.
>
> Also please do some analysis to see if it's a real bug or a false
> positive.  It is a false positive in this case.
>

Dan,
I have already sent v3 of this in which I have used:
sizeof(asma->name) - ASHMEM_NAME_PREFIX_LEN

Thanks!
Simran

> regards,
> dan carpenter
>

[toc] | [prev] | [next] | [standalone]

#1599328

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-03-13 14:00 +0100
Message-ID<tkxJU-Fd-33@gated-at.bofh.it>
In reply to#1599295
On Mon, Mar 13, 2017 at 06:17:22PM +0530, SIMRAN SINGHAL wrote:
> On Mon, Mar 13, 2017 at 6:11 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
> >> Replace strcpy with strlcpy as strcpy does not check for buffer
> >> overflow.
> >> This is found using Flawfinder.
> >>
> >> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> >> ---
> >>  drivers/staging/android/ashmem.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> >> index 7cbad0d..eb2f4ef 100644
> >> --- a/drivers/staging/android/ashmem.c
> >> +++ b/drivers/staging/android/ashmem.c
> >> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
> >>       if (unlikely(asma->file))
> >>               ret = -EINVAL;
> >>       else
> >> -             strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
> >> +             strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
> >> +                     sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This isn't right.
> >
> > Also please do some analysis to see if it's a real bug or a false
> > positive.  It is a false positive in this case.
> >
> 
> Dan,
> I have already sent v3 of this in which I have used:
> sizeof(asma->name) - ASHMEM_NAME_PREFIX_LEN

Yeah.  I saw that.  It's fine, I suppose but you should have done more
analysis to see if it was a real bug like Al and Greg suggested.  The
changelog should say something like:

"The destination buffer is 12345 bytes long but we're copying a 10000
character string so it can overflow."  Occasionally, I will fudge a
little bit on these changelogs to say that I have looked every where to
determine the size of the source buffer and can't figure it out so this
change makes it easier to audit.  But I try to figure it out generally.

Really tools should be able to show that this code is safe.  They
currently don't so far as I know, but they should.  It's a matter of
waiting a year for Smatch to improve.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1599343

FromSIMRAN SINGHAL <singhalsimran0@gmail.com>
Date2017-03-13 14:20 +0100
Message-ID<tky3g-12m-15@gated-at.bofh.it>
In reply to#1599328
On Mon, Mar 13, 2017 at 6:27 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Mar 13, 2017 at 06:17:22PM +0530, SIMRAN SINGHAL wrote:
>> On Mon, Mar 13, 2017 at 6:11 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
>> >> Replace strcpy with strlcpy as strcpy does not check for buffer
>> >> overflow.
>> >> This is found using Flawfinder.
>> >>
>> >> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>> >> ---
>> >>  drivers/staging/android/ashmem.c | 3 ++-
>> >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
>> >> index 7cbad0d..eb2f4ef 100644
>> >> --- a/drivers/staging/android/ashmem.c
>> >> +++ b/drivers/staging/android/ashmem.c
>> >> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>> >>       if (unlikely(asma->file))
>> >>               ret = -EINVAL;
>> >>       else
>> >> -             strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
>> >> +             strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
>> >> +                     sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
>> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > This isn't right.
>> >
>> > Also please do some analysis to see if it's a real bug or a false
>> > positive.  It is a false positive in this case.
>> >
>>
>> Dan,
>> I have already sent v3 of this in which I have used:
>> sizeof(asma->name) - ASHMEM_NAME_PREFIX_LEN
>
> Yeah.  I saw that.  It's fine, I suppose but you should have done more
> analysis to see if it was a real bug like Al and Greg suggested.  The
> changelog should say something like:
>
> "The destination buffer is 12345 bytes long but we're copying a 10000
> character string so it can overflow."  Occasionally, I will fudge a
> little bit on these changelogs to say that I have looked every where to
> determine the size of the source buffer and can't figure it out so this
> change makes it easier to audit.  But I try to figure it out generally.
>
> Really tools should be able to show that this code is safe.  They
> currently don't so far as I know, but they should.  It's a matter of
> waiting a year for Smatch to improve.
>

Thanks!

Will keep this in mind.

> regards,
> dan carpenter
>

[toc] | [prev] | [next] | [standalone]

#1599310

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-03-13 13:50 +0100
Message-ID<tkxAd-ys-3@gated-at.bofh.it>
In reply to#1598461
On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
> Replace strcpy with strlcpy as strcpy does not check for buffer
> overflow.
> This is found using Flawfinder.
> 
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> ---
>  drivers/staging/android/ashmem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index 7cbad0d..eb2f4ef 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>  	if (unlikely(asma->file))
>  		ret = -EINVAL;
>  	else
> -		strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
> +		strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
> +			sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This isn't right.

Also please do some analysis to see if it's a real bug or a false
positive.  It is a false positive in this case.

regards,
dan carpenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web