[PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[Andy Shevchenko]

Use string_upper() from string helper module instead of open coded variant.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/s390/mm/cmm.c    | 11 ++++-------
 arch/s390/mm/extmem.c | 21 ++++++++++++---------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/s390/mm/cmm.c b/arch/s390/mm/cmm.c
index 1141c8d5c0d0..2203164b39da 100644
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -14,8 +14,8 @@
 #include <linux/moduleparam.h>
 #include <linux/gfp.h>
 #include <linux/sched.h>
+#include <linux/string_helpers.h>
 #include <linux/sysctl.h>
-#include <linux/ctype.h>
 #include <linux/swap.h>
 #include <linux/kthread.h>
 #include <linux/oom.h>
@@ -394,13 +394,10 @@ static int __init cmm_init(void)
 		goto out_sysctl;
 #ifdef CONFIG_CMM_IUCV
 	/* convert sender to uppercase characters */
-	if (sender) {
-		int len = strlen(sender);
-		while (len--)
-			sender[len] = toupper(sender[len]);
-	} else {
+	if (sender)
+		string_upper(sender, sender);
+	else
 		sender = cmm_default_sender;
-	}
 
 	rc = smsg_register_callback(SMSG_PREFIX, cmm_smsg_target);
 	if (rc < 0)
diff --git a/arch/s390/mm/extmem.c b/arch/s390/mm/extmem.c
index 5060956b8e7d..2c8c5dc52472 100644
--- a/arch/s390/mm/extmem.c
+++ b/arch/s390/mm/extmem.c
@@ -12,12 +12,12 @@
 
 #include <linux/kernel.h>
 #include <linux/string.h>
+#include <linux/string_helpers.h>
 #include <linux/spinlock.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <linux/memblock.h>
-#include <linux/ctype.h>
 #include <linux/ioport.h>
 #include <linux/refcount.h>
 #include <linux/pgtable.h>
@@ -89,15 +89,18 @@ static int segext_scode = DCSS_SEGEXTX;
 static void
 dcss_mkname(char *name, char *dcss_name)
 {
+	/* Segment name is limited by 8 characters + NUL */
+	char tmp[8 + 1];
 	int i;
 
-	for (i = 0; i < 8; i++) {
-		if (name[i] == '\0')
-			break;
-		dcss_name[i] = toupper(name[i]);
-	}
-	for (; i < 8; i++)
-		dcss_name[i] = ' ';
+	/*
+	 * This snprintf() call does two things:
+	 * - makes a NUL-terminated copy of the input string
+	 * - pads it with spaces
+	 */
+	snprintf(tmp, sizeof(tmp), "%s        ", name);
+	string_upper(dcss_name, tmp);
+
 	ASCEBC(dcss_name, 8);
 }
 
@@ -109,7 +112,7 @@ dcss_mkname(char *name, char *dcss_name)
 static struct dcss_segment *
 segment_by_name (char *name)
 {
-	char dcss_name[9];
+	char dcss_name[8];
 	struct list_head *l;
 	struct dcss_segment *tmp, *retval = NULL;
 
-- 
2.33.0

Re: [PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[Heiko Carstens]

On Fri, Oct 01, 2021 at 04:02:01PM +0300, Andy Shevchenko wrote:
> Use string_upper() from string helper module instead of open coded variant.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/s390/mm/cmm.c    | 11 ++++-------
>  arch/s390/mm/extmem.c | 21 ++++++++++++---------
>  2 files changed, 16 insertions(+), 16 deletions(-)
...
>  static void
>  dcss_mkname(char *name, char *dcss_name)
>  {
> +	/* Segment name is limited by 8 characters + NUL */
> +	char tmp[8 + 1];
>  	int i;
>  
> -	for (i = 0; i < 8; i++) {
> -		if (name[i] == '\0')
> -			break;
> -		dcss_name[i] = toupper(name[i]);
> -	}
> -	for (; i < 8; i++)
> -		dcss_name[i] = ' ';
> +	/*
> +	 * This snprintf() call does two things:
> +	 * - makes a NUL-terminated copy of the input string
> +	 * - pads it with spaces
> +	 */
> +	snprintf(tmp, sizeof(tmp), "%s        ", name);

I can't say I like code where I have to count spaces in order to
verify if the code is actually correct.

> +	string_upper(dcss_name, tmp);
...
>  static struct dcss_segment *
>  segment_by_name (char *name)
>  {
> -	char dcss_name[9];
> +	char dcss_name[8];

string_upper will copy the terminating NUL-byte. By reducing the size
of dcss_name to 8 bytes this will result in stack corruption.

Re: [PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[Andy Shevchenko]

On Mon, Oct 04, 2021 at 10:31:46PM +0200, Heiko Carstens wrote:
> On Fri, Oct 01, 2021 at 04:02:01PM +0300, Andy Shevchenko wrote:

...

> > +	/* Segment name is limited by 8 characters + NUL */
> > +	char tmp[8 + 1];
> >  	int i;
> >  
> > -	for (i = 0; i < 8; i++) {
> > -		if (name[i] == '\0')
> > -			break;
> > -		dcss_name[i] = toupper(name[i]);
> > -	}
> > -	for (; i < 8; i++)
> > -		dcss_name[i] = ' ';
> > +	/*
> > +	 * This snprintf() call does two things:
> > +	 * - makes a NUL-terminated copy of the input string
> > +	 * - pads it with spaces
> > +	 */
> > +	snprintf(tmp, sizeof(tmp), "%s        ", name);
> 
> I can't say I like code where I have to count spaces in order to
> verify if the code is actually correct.

I understand your point, but have any idea how to make it differently
and not ugly at the same time?

> > +	string_upper(dcss_name, tmp);

...

> >  static struct dcss_segment *
> >  segment_by_name (char *name)
> >  {
> > -	char dcss_name[9];
> > +	char dcss_name[8];
> 
> string_upper will copy the terminating NUL-byte. By reducing the size
> of dcss_name to 8 bytes this will result in stack corruption.

Nope. Even in the original code this additional byte is left unused.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[Heiko Carstens]

On Tue, Oct 05, 2021 at 11:18:38AM +0300, Andy Shevchenko wrote:
> On Mon, Oct 04, 2021 at 10:31:46PM +0200, Heiko Carstens wrote:
> > On Fri, Oct 01, 2021 at 04:02:01PM +0300, Andy Shevchenko wrote:
> > > +	/* Segment name is limited by 8 characters + NUL */
> > > +	char tmp[8 + 1];
> > >  	int i;
> > >  
> > > -	for (i = 0; i < 8; i++) {
> > > -		if (name[i] == '\0')
> > > -			break;
> > > -		dcss_name[i] = toupper(name[i]);
> > > -	}
> > > -	for (; i < 8; i++)
> > > -		dcss_name[i] = ' ';
> > > +	/*
> > > +	 * This snprintf() call does two things:
> > > +	 * - makes a NUL-terminated copy of the input string
> > > +	 * - pads it with spaces
> > > +	 */
> > > +	snprintf(tmp, sizeof(tmp), "%s        ", name);
> > 
> > I can't say I like code where I have to count spaces in order to
> > verify if the code is actually correct.
> 
> I understand your point, but have any idea how to make it differently
> and not ugly at the same time?

Don't know. You could use strncopy+strlen+memset (with space
character). After all I'm not very convinced that the resulting code
buys us anything compared to the current variant.

> > > +	string_upper(dcss_name, tmp);
> 
> ...
> 
> > >  static struct dcss_segment *
> > >  segment_by_name (char *name)
> > >  {
> > > -	char dcss_name[9];
> > > +	char dcss_name[8];
> > 
> > string_upper will copy the terminating NUL-byte. By reducing the size
> > of dcss_name to 8 bytes this will result in stack corruption.
> 
> Nope. Even in the original code this additional byte is left unused.

I'm talking about the new code, not the old code: If "name" points to
a NUL terminated eight chararacter string, then the new code will use
snprintf to copy it 1:1 to tmp, and the subsequent string_upper() will
copy the string (upper cased) to dcss_name, now including the NUL
terminating byte, which won't fit into dcss_name.
Am I missing something here?

Re: [PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[Andy Shevchenko]

On Tue, Oct 05, 2021 at 10:54:28AM +0200, Heiko Carstens wrote:
> On Tue, Oct 05, 2021 at 11:18:38AM +0300, Andy Shevchenko wrote:
> > On Mon, Oct 04, 2021 at 10:31:46PM +0200, Heiko Carstens wrote:
> > > On Fri, Oct 01, 2021 at 04:02:01PM +0300, Andy Shevchenko wrote:

...

> > > > +	char tmp[8 + 1];
> > > >  	int i;
> > > >  
> > > > -	for (i = 0; i < 8; i++) {
> > > > -		if (name[i] == '\0')
> > > > -			break;
> > > > -		dcss_name[i] = toupper(name[i]);
> > > > -	}
> > > > -	for (; i < 8; i++)
> > > > -		dcss_name[i] = ' ';
> > > > +	/*
> > > > +	 * This snprintf() call does two things:
> > > > +	 * - makes a NUL-terminated copy of the input string
> > > > +	 * - pads it with spaces
> > > > +	 */
> > > > +	snprintf(tmp, sizeof(tmp), "%s        ", name);
> > > 
> > > I can't say I like code where I have to count spaces in order to
> > > verify if the code is actually correct.
> > 
> > I understand your point, but have any idea how to make it differently
> > and not ugly at the same time?
> 
> Don't know. You could use strncopy+strlen+memset (with space
> character). After all I'm not very convinced that the resulting code
> buys us anything compared to the current variant.

Yup, so let's convert only the first part then.

...

> > > > -	char dcss_name[9];
> > > > +	char dcss_name[8];
> > > 
> > > string_upper will copy the terminating NUL-byte. By reducing the size
> > > of dcss_name to 8 bytes this will result in stack corruption.
> > 
> > Nope. Even in the original code this additional byte is left unused.
> 
> I'm talking about the new code, not the old code: If "name" points to
> a NUL terminated eight chararacter string, then the new code will use
> snprintf to copy it 1:1 to tmp, and the subsequent string_upper() will
> copy the string (upper cased) to dcss_name, now including the NUL
> terminating byte, which won't fit into dcss_name.
> Am I missing something here?

Ah, indeed, although it's rather bug in the implementation of above.
But original code has it not in use.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[Heiko Carstens]

On Fri, Oct 01, 2021 at 04:02:01PM +0300, Andy Shevchenko wrote:
> Use string_upper() from string helper module instead of open coded variant.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/s390/mm/cmm.c    | 11 ++++-------
>  arch/s390/mm/extmem.c | 21 ++++++++++++---------
>  2 files changed, 16 insertions(+), 16 deletions(-)

Applied, but as discussed only the cmm part.

RE: [PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[David Laight]

...
> > +	 * This snprintf() call does two things:
> > +	 * - makes a NUL-terminated copy of the input string
> > +	 * - pads it with spaces
> > +	 */
> > +	snprintf(tmp, sizeof(tmp), "%s        ", name);
> 
> I can't say I like code where I have to count spaces in order to
> verify if the code is actually correct.

What it wrong with "%-8.8s" ?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[Heiko Carstens]

On Mon, Oct 11, 2021 at 08:21:15AM +0000, David Laight wrote:
> ...
> > > +	 * This snprintf() call does two things:
> > > +	 * - makes a NUL-terminated copy of the input string
> > > +	 * - pads it with spaces
> > > +	 */
> > > +	snprintf(tmp, sizeof(tmp), "%s        ", name);
> > 
> > I can't say I like code where I have to count spaces in order to
> > verify if the code is actually correct.
> 
> What it wrong with "%-8.8s" ?

There's nothing wrong with it, except lack of imagination on my side ;)
Andy, care to to send a separate patch just for extmem.c?

RE: [PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[David Laight]

From: Heiko Carstens
> Sent: 11 October 2021 11:10
> 
> On Mon, Oct 11, 2021 at 08:21:15AM +0000, David Laight wrote:
> > ...
> > > > +	 * This snprintf() call does two things:
> > > > +	 * - makes a NUL-terminated copy of the input string
> > > > +	 * - pads it with spaces
> > > > +	 */
> > > > +	snprintf(tmp, sizeof(tmp), "%s        ", name);
> > >
> > > I can't say I like code where I have to count spaces in order to
> > > verify if the code is actually correct.
> >
> > What it wrong with "%-8.8s" ?
> 
> There's nothing wrong with it, except lack of imagination on my side ;)
> Andy, care to to send a separate patch just for extmem.c?

Are any of the snprintf() versions actually correct at all?
The implication of the comment is that the input string might
not be NUL terminated - in which case it shouldn't be passed
to snprintf().
I don't think you can assume that the format processing doesn't
do a strlen() of any %s argument - even if a maximum field
width is given.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v1 1/1] s390: Use string_upper() instead of open coded variant

[Heiko Carstens]

On Tue, Oct 12, 2021 at 08:04:58AM +0000, David Laight wrote:
> From: Heiko Carstens
> > Sent: 11 October 2021 11:10
> > 
> > On Mon, Oct 11, 2021 at 08:21:15AM +0000, David Laight wrote:
> > > ...
> > > > > +	 * This snprintf() call does two things:
> > > > > +	 * - makes a NUL-terminated copy of the input string
> > > > > +	 * - pads it with spaces
> > > > > +	 */
> > > > > +	snprintf(tmp, sizeof(tmp), "%s        ", name);
> > > >
> > > > I can't say I like code where I have to count spaces in order to
> > > > verify if the code is actually correct.
> > >
> > > What it wrong with "%-8.8s" ?
> > 
> > There's nothing wrong with it, except lack of imagination on my side ;)
> > Andy, care to to send a separate patch just for extmem.c?
> 
> Are any of the snprintf() versions actually correct at all?
> The implication of the comment is that the input string might
> not be NUL terminated - in which case it shouldn't be passed
> to snprintf().
> I don't think you can assume that the format processing doesn't
> do a strlen() of any %s argument - even if a maximum field
> width is given.

The input string is a NUL terminated ASCII string. The output string
is not. It is used to communicate with a hypervisor, which expects an
eight character EBCDIC non NUL terminated name, where the name is
either eight characters long or filled up with spaces.
So using snprintf here should be fine. On the other hand I really
don't see a pressing need to change anything here.