Re: [PATCH] arch: s390: appldata: using strncpy() instead of sprintf()

Re: [PATCH] arch: s390: appldata: using strncpy() instead of sprintf()

[Geert Uytterhoeven]

On Mon, May 27, 2013 at 4:59 AM, Chen Gang <gang.chen@asianux.com> wrote:
> 'buf[2]' is 2 bytes length, and sprintf() will append '\0' at the end
> of string "?\n", so original implementation is memory overflow.

Nice catch!

> Need use strncpy() instead of sprintf().

Or memcpy(), as the string is always 2 bytes?
Of course that would break future extension with multi-digit responses, but in
that case, the buffer size needs to be expanded, too.

Anyway, this code needed some more brain cycles to actually understand
what's it doing, and whether it's correct. So I guess it's ripe for more
refactoring...

> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/s390/appldata/appldata_base.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
> index bae0f40..566ea87 100644
> --- a/arch/s390/appldata/appldata_base.c
> +++ b/arch/s390/appldata/appldata_base.c
> @@ -212,7 +212,8 @@ appldata_timer_handler(ctl_table *ctl, int write,
>                 return 0;
>         }
>         if (!write) {
> -               len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");

Before, len was always 2. The string was zero-terminated but the zero was
not returned to user space, which is OK.

> +               len = strncpy(buf, appldata_timer_active ? "1\n" : "0\n",
> +                                                               sizeof(buf));

Oops, strncpy() doesn't return a size, but a pointer! So this doesn't work.
I guess you haven't compiled this?
http://kernel.org/pub/tools/crosstool/files/bin/

>                 if (len > *lenp)
>                         len = *lenp;
>                 if (copy_to_user(buffer, buf, len))

What about creating a "uprintf()" that formats strings to a userspace buffer?

> @@ -317,7 +318,7 @@ appldata_generic_handler(ctl_table *ctl, int write,
>                 return 0;
>         }
>         if (!write) {
> -               len = sprintf(buf, ops->active ? "1\n" : "0\n");
> +               len = strncpy(buf, ops->active ? "1\n" : "0\n", sizeof(buf));

Same here.

>                 if (len > *lenp)
>                         len = *lenp;
>                 if (copy_to_user(buffer, buf, len)) {

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] arch: s390: appldata: using strncpy() instead of sprintf()

[Chen Gang]

On 05/27/2013 04:34 PM, Geert Uytterhoeven wrote:
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> > ---
>> >  arch/s390/appldata/appldata_base.c |    5 +++--
>> >  1 files changed, 3 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
>> > index bae0f40..566ea87 100644
>> > --- a/arch/s390/appldata/appldata_base.c
>> > +++ b/arch/s390/appldata/appldata_base.c
>> > @@ -212,7 +212,8 @@ appldata_timer_handler(ctl_table *ctl, int write,
>> >                 return 0;
>> >         }
>> >         if (!write) {
>> > -               len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
> Before, len was always 2. The string was zero-terminated but the zero was
> not returned to user space, which is OK.
> 

I guess, the original API did not treat it as a zero-terminated string,
it is more like a 'protocol' contents for 'write' or '!write' between
kernel mode and user mode.

At least the kernel did not treat it as a zero-terminated string, we can
reference the '!write' work flow to know about it.


>> > +               len = strncpy(buf, appldata_timer_active ? "1\n" : "0\n",
>> > +                                                               sizeof(buf));
> Oops, strncpy() doesn't return a size, but a pointer! So this doesn't work.
> I guess you haven't compiled this?
> http://kernel.org/pub/tools/crosstool/files/bin/
> 

Oh, it is my fault, I need compile it for a basic checking.

>> >                 if (len > *lenp)
>> >                         len = *lenp;
>> >                 if (copy_to_user(buffer, buf, len))
> What about creating a "uprintf()" that formats strings to a userspace buffer?
> 

Hmm... maybe it is a good idea for processing the zero-terminated string
from kernel mode to user mode, but I guess, for our case, it is not a
zero-terminated string, so...


All together, thank you very much for your checking, I should send patch v2.


Thanks.
-- 
Chen Gang

Asianux Corporation

[PATCH] arch: s390: include: asm: typo issue for the redundency comma, found by cross compiling.

[Chen Gang]

It is a typo issue, remove the redundancy comma.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/s390/include/asm/pgtable.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 66b52d0..79346dc 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1351,7 +1351,7 @@ static inline pmd_t pmd_mkwrite(pmd_t pmd)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 
 #define __HAVE_ARCH_PGTABLE_DEPOSIT
-extern void pgtable_trans_huge_deposit(struct mm_struct *mm, , pmd_t *pmdp,
+extern void pgtable_trans_huge_deposit(struct mm_struct *mm, pmd_t *pmdp,
 				       pgtable_t pgtable);
 
 #define __HAVE_ARCH_PGTABLE_WITHDRAW
-- 
1.7.7.6

[PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Chen Gang]

'buf[2]' is 2 bytes length, and sprintf() will append '\0' at the end
of string "?\n", so original implementation is memory overflow.

Need use strncpy() and strnlen() instead of sprintf().


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/s390/appldata/appldata_base.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
index bae0f40..87a2209 100644
--- a/arch/s390/appldata/appldata_base.c
+++ b/arch/s390/appldata/appldata_base.c
@@ -212,7 +212,9 @@ appldata_timer_handler(ctl_table *ctl, int write,
 		return 0;
 	}
 	if (!write) {
-		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
+		strncpy(buf, appldata_timer_active ? "1\n" : "0\n",
+			ARRAY_SIZE(buf));
+		len = strnlen(buf, ARRAY_SIZE(buf));
 		if (len > *lenp)
 			len = *lenp;
 		if (copy_to_user(buffer, buf, len))
@@ -317,7 +319,8 @@ appldata_generic_handler(ctl_table *ctl, int write,
 		return 0;
 	}
 	if (!write) {
-		len = sprintf(buf, ops->active ? "1\n" : "0\n");
+		strncpy(buf, ops->active ? "1\n" : "0\n", ARRAY_SIZE(buf));
+		len = strnlen(buf, ARRAY_SIZE(buf));
 		if (len > *lenp)
 			len = *lenp;
 		if (copy_to_user(buffer, buf, len)) {
-- 
1.7.7.6

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Gerald Schaefer]

On Mon, 27 May 2013 17:55:13 +0800
Chen Gang <gang.chen@asianux.com> wrote:

> 
> 'buf[2]' is 2 bytes length, and sprintf() will append '\0' at the end
> of string "?\n", so original implementation is memory overflow.
> 
> Need use strncpy() and strnlen() instead of sprintf().
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

Applied, thanks.

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Chen Gang]

On 05/28/2013 12:23 AM, Gerald Schaefer wrote:
> On Mon, 27 May 2013 17:55:13 +0800
> Chen Gang <gang.chen@asianux.com> wrote:
> 
>> > 
>> > 'buf[2]' is 2 bytes length, and sprintf() will append '\0' at the end
>> > of string "?\n", so original implementation is memory overflow.
>> > 
>> > Need use strncpy() and strnlen() instead of sprintf().
>> > 
>> > 
>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Applied, thanks.
> 

Thank you, too.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[walter harms]

Am 27.05.2013 11:55, schrieb Chen Gang:
> 
> 'buf[2]' is 2 bytes length, and sprintf() will append '\0' at the end
> of string "?\n", so original implementation is memory overflow.
> 
> Need use strncpy() and strnlen() instead of sprintf().
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/s390/appldata/appldata_base.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
> index bae0f40..87a2209 100644
> --- a/arch/s390/appldata/appldata_base.c
> +++ b/arch/s390/appldata/appldata_base.c
> @@ -212,7 +212,9 @@ appldata_timer_handler(ctl_table *ctl, int write,
>  		return 0;
>  	}
>  	if (!write) {
> -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
> +		strncpy(buf, appldata_timer_active ? "1\n" : "0\n",
> +			ARRAY_SIZE(buf));

an other way would be

buf[]="0\n";
if (appldata_timer_active) buf[0]='1';


> +		len = strnlen(buf, ARRAY_SIZE(buf));

can len ever change ?


re,
 wh



>  		if (len > *lenp)
>  			len = *lenp;
>  		if (copy_to_user(buffer, buf, len))
> @@ -317,7 +319,8 @@ appldata_generic_handler(ctl_table *ctl, int write,
>  		return 0;
>  	}
>  	if (!write) {
> -		len = sprintf(buf, ops->active ? "1\n" : "0\n");
> +		strncpy(buf, ops->active ? "1\n" : "0\n", ARRAY_SIZE(buf));
> +		len = strnlen(buf, ARRAY_SIZE(buf));
>  		if (len > *lenp)
>  			len = *lenp;
>  		if (copy_to_user(buffer, buf, len)) {

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Chen Gang]

On 05/28/2013 12:53 AM, walter harms wrote:
> Am 27.05.2013 11:55, schrieb Chen Gang:
>> diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
>> index bae0f40..87a2209 100644
>> --- a/arch/s390/appldata/appldata_base.c
>> +++ b/arch/s390/appldata/appldata_base.c
>> @@ -212,7 +212,9 @@ appldata_timer_handler(ctl_table *ctl, int write,
>>  		return 0;
>>  	}
>>  	if (!write) {
>> -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
>> +		strncpy(buf, appldata_timer_active ? "1\n" : "0\n",
>> +			ARRAY_SIZE(buf));
> 
> an other way would be
> 
> buf[]="0\n";
> if (appldata_timer_active) buf[0]='1';
> 
> 
>> +		len = strnlen(buf, ARRAY_SIZE(buf));
> 
> can len ever change ?
> 

I do not know whether it will be changed, but we need treat it that it
may be changed in the future.

I guess it uses "?\n" as a protocol between kernel mode and user mode.
This patch fix the 'protocol' transferring issue independent of the
'protocol' itself.

In the future, the 'protocol' updates to new version (e.g. append
additional characters, which will still compitable with the original
version), what this patch have done will be still effective.


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[David Howells]

Chen Gang <gang.chen@asianux.com> wrote:

> -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
> +		strncpy(buf, appldata_timer_active ? "1\n" : "0\n",
> +			ARRAY_SIZE(buf));
> +		len = strnlen(buf, ARRAY_SIZE(buf));

Since the strings here are a fixed, preknown size, you should use memcpy (or
just fill in the array directly which would likely take fewer instructions
since the strings are so short and vary by one character) and hard-code the
length or use sizeof() instead of strnlen().

David

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Chen Gang]

On 05/28/2013 06:22 PM, David Howells wrote:
> Chen Gang <gang.chen@asianux.com> wrote:
> 
>> > -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
>> > +		strncpy(buf, appldata_timer_active ? "1\n" : "0\n",
>> > +			ARRAY_SIZE(buf));
>> > +		len = strnlen(buf, ARRAY_SIZE(buf));
> Since the strings here are a fixed, preknown size, you should use memcpy (or
> just fill in the array directly which would likely take fewer instructions
> since the strings are so short and vary by one character) and hard-code the
> length or use sizeof() instead of strnlen().

I treat it "?\n" as a 'protocol' between kernel mode and user mode, the
original issue is about transferring 'protocol' data. the patch wants to
fix the issue, independent of 'protocol'.

appldata_timer_handler() itself has already separated "transferring
'protocol' data" from "processing 'protocol' data" (but not let the
"processing 'protocol' data" in another new function)

Your suggestion will improve the speed, but may merge "transferring
'protocol' data" and "processing 'protocol' data" together.

So if the performance is not quite important in this location, it is
still reasonable to follow with the original author's willing
(especially what he/she has done sounds reasonable too).


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[David Howells]

Chen Gang <gang.chen@asianux.com> wrote:

> Your suggestion will improve the speed, but may merge "transferring
> 'protocol' data" and "processing 'protocol' data" together.

Look at it this way: You're having to step very carefully because you are
fully expecting the strings not to be NUL-terminated.  Therefore you probably
avoid using string functions if you can.

In fact, looking at the code, why are you copying the data through an
intermediate buffer at all?  Why not just copy directly to userspace:

	int len;
	char buf[2];

	if (!*lenp || *ppos) {
		*lenp = 0;
		return 0;
	}
	if (!write) {
-		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
+		const char *ptr = appldata_timer_active ? "1\n" : "0\n";
+		size_t len = 2;
		if (len > *lenp)
			len = *lenp;
		if (copy_to_user(buffer, buf, len))
			return -EFAULT;
		goto out;
	}

Put like that, it's fairly obvious what is going on.

David

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Gerald Schaefer]

On Tue, 28 May 2013 13:17:09 +0100
David Howells <dhowells@redhat.com> wrote:

> Chen Gang <gang.chen@asianux.com> wrote:
> 
> > Your suggestion will improve the speed, but may merge "transferring
> > 'protocol' data" and "processing 'protocol' data" together.
> 
> Look at it this way: You're having to step very carefully because you are
> fully expecting the strings not to be NUL-terminated.  Therefore you probably
> avoid using string functions if you can.
> 
> In fact, looking at the code, why are you copying the data through an
> intermediate buffer at all?  Why not just copy directly to userspace:
> 
> 	int len;
> 	char buf[2];
> 
> 	if (!*lenp || *ppos) {
> 		*lenp = 0;
> 		return 0;
> 	}
> 	if (!write) {
> -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
> +		const char *ptr = appldata_timer_active ? "1\n" : "0\n";
> +		size_t len = 2;
> 		if (len > *lenp)
> 			len = *lenp;
> 		if (copy_to_user(buffer, buf, len))
> 			return -EFAULT;
> 		goto out;
> 	}
> 
> Put like that, it's fairly obvious what is going on.

Yes, we could do that for !write, but we can't get rid of the buffer
completely, as we need it for the other case and copy_from_user().
I have already applied the v2 version from Chen Gang, as it fixes the
overflow bug, and the affected code is not performance critical at all.
I like the improved readability of your approach, but the patch is already
on its way to Martins "for-linus" branch, and I think it's "good enough".

Thanks for all the feedback to this little piece of very old (and partly
ugly) code from my very first Linux device driver :-)

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Chen Gang]

On 05/29/2013 12:03 AM, Gerald Schaefer wrote:
> On Tue, 28 May 2013 13:17:09 +0100
> David Howells <dhowells@redhat.com> wrote:
> 
>> > Chen Gang <gang.chen@asianux.com> wrote:
>> > 
>>> > > Your suggestion will improve the speed, but may merge "transferring
>>> > > 'protocol' data" and "processing 'protocol' data" together.
>> > 
>> > Look at it this way: You're having to step very carefully because you are
>> > fully expecting the strings not to be NUL-terminated.  Therefore you probably
>> > avoid using string functions if you can.
>> > 
>> > In fact, looking at the code, why are you copying the data through an
>> > intermediate buffer at all?  Why not just copy directly to userspace:
>> > 
>> > 	int len;
>> > 	char buf[2];
>> > 
>> > 	if (!*lenp || *ppos) {
>> > 		*lenp = 0;
>> > 		return 0;
>> > 	}
>> > 	if (!write) {
>> > -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
>> > +		const char *ptr = appldata_timer_active ? "1\n" : "0\n";
>> > +		size_t len = 2;
>> > 		if (len > *lenp)
>> > 			len = *lenp;
>> > 		if (copy_to_user(buffer, buf, len))
>> > 			return -EFAULT;
>> > 		goto out;
>> > 	}
>> > 
>> > Put like that, it's fairly obvious what is going on.
> Yes, we could do that for !write, but we can't get rid of the buffer
> completely, as we need it for the other case and copy_from_user().
> I have already applied the v2 version from Chen Gang, as it fixes the
> overflow bug, and the affected code is not performance critical at all.
> I like the improved readability of your approach, but the patch is already
> on its way to Martins "for-linus" branch, and I think it's "good enough".
> 
> Thanks for all the feedback to this little piece of very old (and partly
> ugly) code from my very first Linux device driver :-)
> 

OK, thank you. and thank all related feedback from Geert, Walter, David


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Chen Gang]

On 05/28/2013 08:17 PM, David Howells wrote:
> Chen Gang <gang.chen@asianux.com> wrote:
> 
>> Your suggestion will improve the speed, but may merge "transferring
>> 'protocol' data" and "processing 'protocol' data" together.
> 
> Look at it this way: You're having to step very carefully because you are
> fully expecting the strings not to be NUL-terminated.  Therefore you probably
> avoid using string functions if you can.
> 
> In fact, looking at the code, why are you copying the data through an
> intermediate buffer at all?  Why not just copy directly to userspace:
> 
> 	int len;
> 	char buf[2];
> 
> 	if (!*lenp || *ppos) {
> 		*lenp = 0;
> 		return 0;
> 	}
> 	if (!write) {
> -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
> +		const char *ptr = appldata_timer_active ? "1\n" : "0\n";
> +		size_t len = 2;
> 		if (len > *lenp)
> 			len = *lenp;
> 		if (copy_to_user(buffer, buf, len))

may: "if (copy_to_user(buffer, ptr, len))" ?

:-)

> 			return -EFAULT;
> 		goto out;
> 	}
> 
> Put like that, it's fairly obvious what is going on.
> 

It seems well fixed (still independent of processing 'protocol' data).

:-)


And how about below:

-------------------------------diff begin------------------------------


diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
index bae0f40..27f200d 100644
--- a/arch/s390/appldata/appldata_base.c
+++ b/arch/s390/appldata/appldata_base.c
@@ -212,10 +212,9 @@ appldata_timer_handler(ctl_table *ctl, int write,
 		return 0;
 	}
 	if (!write) {
-		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
-		if (len > *lenp)
-			len = *lenp;
-		if (copy_to_user(buffer, buf, len))
+		if (copy_to_user(buffer,
+				 appldata_timer_active ? "1\n" : "0\n",
+				 min(2, *lenp))
 			return -EFAULT;
 		goto out;
 	}

-------------------------------diff end--------------------------------


Thanks
-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Dan Carpenter]

On Wed, May 29, 2013 at 09:28:43AM +0800, Chen Gang wrote:

> diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
> index bae0f40..27f200d 100644
> --- a/arch/s390/appldata/appldata_base.c
> +++ b/arch/s390/appldata/appldata_base.c
> @@ -212,10 +212,9 @@ appldata_timer_handler(ctl_table *ctl, int write,
>  		return 0;
>  	}
>  	if (!write) {
> -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
> -		if (len > *lenp)
> -			len = *lenp;
> -		if (copy_to_user(buffer, buf, len))
> +		if (copy_to_user(buffer,
> +				 appldata_timer_active ? "1\n" : "0\n",
> +				 min(2, *lenp))

I don't have a cross compiler set up, but this will generate a
warning, I think.  min_t() is needed.

regards,
dan carpenter

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Chen Gang]

On 05/29/2013 02:30 PM, Dan Carpenter wrote:
> On Wed, May 29, 2013 at 09:28:43AM +0800, Chen Gang wrote:
> 
>> > diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
>> > index bae0f40..27f200d 100644
>> > --- a/arch/s390/appldata/appldata_base.c
>> > +++ b/arch/s390/appldata/appldata_base.c
>> > @@ -212,10 +212,9 @@ appldata_timer_handler(ctl_table *ctl, int write,
>> >  		return 0;
>> >  	}
>> >  	if (!write) {
>> > -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
>> > -		if (len > *lenp)
>> > -			len = *lenp;
>> > -		if (copy_to_user(buffer, buf, len))
>> > +		if (copy_to_user(buffer,
>> > +				 appldata_timer_active ? "1\n" : "0\n",
>> > +				 min(2, *lenp))
> I don't have a cross compiler set up, but this will generate a
> warning, I think.  min_t() is needed.

Yes, it even can not pass compiling: need additional ')' for 'if'.

The new fix is below, (for pass compiling, also need fix the incorrect
declaration, which I have already sent an individual patch for it).

-----------------------------diff v2 begin------------------------------

diff --git a/arch/s390/appldata/appldata_base.c
b/arch/s390/appldata/appldata_base.c
index bae0f40..b0915b4 100644
--- a/arch/s390/appldata/appldata_base.c
+++ b/arch/s390/appldata/appldata_base.c
@@ -212,10 +212,9 @@ appldata_timer_handler(ctl_table *ctl, int write,
 		return 0;
 	}
 	if (!write) {
-		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
-		if (len > *lenp)
-			len = *lenp;
-		if (copy_to_user(buffer, buf, len))
+		if (copy_to_user(buffer,
+				 appldata_timer_active ? "1\n" : "0\n",
+				 min_t(size_t, 2, *lenp)))
 			return -EFAULT;
 		goto out;
 	}
diff --git a/arch/s390/include/asm/pgtable.h
b/arch/s390/include/asm/pgtable.h
index 172209c..ed88890 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1348,7 +1348,7 @@ static inline pmd_t pmd_mkwrite(pmd_t pmd)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE

 #define __HAVE_ARCH_PGTABLE_DEPOSIT
-extern void pgtable_trans_huge_deposit(struct mm_struct *mm, , pmd_t *pmdp,
+extern void pgtable_trans_huge_deposit(struct mm_struct *mm, pmd_t *pmdp,
 				       pgtable_t pgtable);

 #define __HAVE_ARCH_PGTABLE_WITHDRAW


-----------------------------diff v2 end--------------------------------


For cross compiler, I use fedora 16/17, can yum update binutils-*
to get many platform cross-compilers (s390, powerpc, arm, xtensa, ...).


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] arch: s390: appldata: using strncpy() and strnlen() instead of sprintf()

[Chen Gang]

On 05/29/2013 03:29 PM, Chen Gang wrote:
> On 05/29/2013 02:30 PM, Dan Carpenter wrote:
>> On Wed, May 29, 2013 at 09:28:43AM +0800, Chen Gang wrote:
>>
>>>> diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
>>>> index bae0f40..27f200d 100644
>>>> --- a/arch/s390/appldata/appldata_base.c
>>>> +++ b/arch/s390/appldata/appldata_base.c
>>>> @@ -212,10 +212,9 @@ appldata_timer_handler(ctl_table *ctl, int write,
>>>>  		return 0;
>>>>  	}
>>>>  	if (!write) {
>>>> -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
>>>> -		if (len > *lenp)
>>>> -			len = *lenp;
>>>> -		if (copy_to_user(buffer, buf, len))
>>>> +		if (copy_to_user(buffer,
>>>> +				 appldata_timer_active ? "1\n" : "0\n",
>>>> +				 min(2, *lenp))
>> I don't have a cross compiler set up, but this will generate a
>> warning, I think.  min_t() is needed.
> 
> Yes, it even can not pass compiling: need additional ')' for 'if'.
> 
> The new fix is below, (for pass compiling, also need fix the incorrect
> declaration, which I have already sent an individual patch for it).
> 

Excuse me, the diff v2 is word wrap, if really need it, please edit it.

If necessary, I will send the related patch for it.

Thanks.

> -----------------------------diff v2 begin------------------------------
> 
> diff --git a/arch/s390/appldata/appldata_base.c
> b/arch/s390/appldata/appldata_base.c
> index bae0f40..b0915b4 100644
> --- a/arch/s390/appldata/appldata_base.c
> +++ b/arch/s390/appldata/appldata_base.c
> @@ -212,10 +212,9 @@ appldata_timer_handler(ctl_table *ctl, int write,
>  		return 0;
>  	}
>  	if (!write) {
> -		len = sprintf(buf, appldata_timer_active ? "1\n" : "0\n");
> -		if (len > *lenp)
> -			len = *lenp;
> -		if (copy_to_user(buffer, buf, len))
> +		if (copy_to_user(buffer,
> +				 appldata_timer_active ? "1\n" : "0\n",
> +				 min_t(size_t, 2, *lenp)))
>  			return -EFAULT;
>  		goto out;
>  	}
> diff --git a/arch/s390/include/asm/pgtable.h
> b/arch/s390/include/asm/pgtable.h
> index 172209c..ed88890 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> @@ -1348,7 +1348,7 @@ static inline pmd_t pmd_mkwrite(pmd_t pmd)
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> 
>  #define __HAVE_ARCH_PGTABLE_DEPOSIT
> -extern void pgtable_trans_huge_deposit(struct mm_struct *mm, , pmd_t *pmdp,
> +extern void pgtable_trans_huge_deposit(struct mm_struct *mm, pmd_t *pmdp,
>  				       pgtable_t pgtable);
> 
>  #define __HAVE_ARCH_PGTABLE_WITHDRAW
> 
> 
> -----------------------------diff v2 end--------------------------------
> 
> 
> For cross compiler, I use fedora 16/17, can yum update binutils-*
> to get many platform cross-compilers (s390, powerpc, arm, xtensa, ...).
> 
> 
> Thanks.
> 


-- 
Chen Gang

Asianux Corporation