linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/1] vsprintf: Fix potential unaligned access
@ 2022-01-10 20:50 Andy Shevchenko
  2022-01-10 22:12 ` Sakari Ailus
  2022-01-11 14:57 ` Petr Mladek
  0 siblings, 2 replies; 9+ messages in thread
From: Andy Shevchenko @ 2022-01-10 20:50 UTC (permalink / raw)
  To: Petr Mladek, linux-kernel
  Cc: Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, Nick Desaulniers, Sakari Ailus

The %p4cc specifier in some cases might get an unaligned pointer.
Due to this we need to make copy to local variable once to avoid
potential crashes on some architectures due to improper access.

Fixes: af612e43de6d ("lib/vsprintf: Add support for printing V4L2 and DRM fourccs")
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c130dcaca5e2..b02f01366acb 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -49,6 +49,7 @@
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/byteorder.h>	/* cpu_to_le16 */
+#include <asm/unaligned.h>
 
 #include <linux/string_helpers.h>
 #include "kstrtox.h"
@@ -1761,7 +1762,7 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
 	char output[sizeof("0123 little-endian (0x01234567)")];
 	char *p = output;
 	unsigned int i;
-	u32 val;
+	u32 orig, val;
 
 	if (fmt[1] != 'c' || fmt[2] != 'c')
 		return error_string(buf, end, "(%p4?)", spec);
@@ -1769,21 +1770,22 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
 	if (check_pointer(&buf, end, fourcc, spec))
 		return buf;
 
-	val = *fourcc & ~BIT(31);
+	orig = get_unaligned(fourcc);
+	val = orig & ~BIT(31);
 
-	for (i = 0; i < sizeof(*fourcc); i++) {
+	for (i = 0; i < sizeof(u32); i++) {
 		unsigned char c = val >> (i * 8);
 
 		/* Print non-control ASCII characters as-is, dot otherwise */
 		*p++ = isascii(c) && isprint(c) ? c : '.';
 	}
 
-	strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
+	strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
 	p += strlen(p);
 
 	*p++ = ' ';
 	*p++ = '(';
-	p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, sizeof(u32));
+	p = special_hex_number(p, output + sizeof(output) - 2, orig, sizeof(u32));
 	*p++ = ')';
 	*p = '\0';
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access
  2022-01-10 20:50 [PATCH v1 1/1] vsprintf: Fix potential unaligned access Andy Shevchenko
@ 2022-01-10 22:12 ` Sakari Ailus
  2022-01-11 10:10   ` Andy Shevchenko
  2022-01-11 14:57 ` Petr Mladek
  1 sibling, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2022-01-10 22:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Petr Mladek, linux-kernel, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Nick Desaulniers

Hi Andy,

On Mon, Jan 10, 2022 at 10:50:49PM +0200, Andy Shevchenko wrote:
> The %p4cc specifier in some cases might get an unaligned pointer.
> Due to this we need to make copy to local variable once to avoid
> potential crashes on some architectures due to improper access.

I guess this problem exists virtually everywhere where pointers are being
handled: the pointer could be unaligned. Does this even address the false
positive compiler warning?

> 
> Fixes: af612e43de6d ("lib/vsprintf: Add support for printing V4L2 and DRM fourccs")
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/vsprintf.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index c130dcaca5e2..b02f01366acb 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -49,6 +49,7 @@
>  
>  #include <asm/page.h>		/* for PAGE_SIZE */
>  #include <asm/byteorder.h>	/* cpu_to_le16 */
> +#include <asm/unaligned.h>
>  
>  #include <linux/string_helpers.h>
>  #include "kstrtox.h"
> @@ -1761,7 +1762,7 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
>  	char output[sizeof("0123 little-endian (0x01234567)")];
>  	char *p = output;
>  	unsigned int i;
> -	u32 val;
> +	u32 orig, val;
>  
>  	if (fmt[1] != 'c' || fmt[2] != 'c')
>  		return error_string(buf, end, "(%p4?)", spec);
> @@ -1769,21 +1770,22 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
>  	if (check_pointer(&buf, end, fourcc, spec))
>  		return buf;
>  
> -	val = *fourcc & ~BIT(31);
> +	orig = get_unaligned(fourcc);
> +	val = orig & ~BIT(31);
>  
> -	for (i = 0; i < sizeof(*fourcc); i++) {
> +	for (i = 0; i < sizeof(u32); i++) {
>  		unsigned char c = val >> (i * 8);
>  
>  		/* Print non-control ASCII characters as-is, dot otherwise */
>  		*p++ = isascii(c) && isprint(c) ? c : '.';
>  	}
>  
> -	strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
> +	strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
>  	p += strlen(p);
>  
>  	*p++ = ' ';
>  	*p++ = '(';
> -	p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, sizeof(u32));
> +	p = special_hex_number(p, output + sizeof(output) - 2, orig, sizeof(u32));
>  	*p++ = ')';
>  	*p = '\0';
>  

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access
  2022-01-10 22:12 ` Sakari Ailus
@ 2022-01-11 10:10   ` Andy Shevchenko
  2022-01-11 10:54     ` Sakari Ailus
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2022-01-11 10:10 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Petr Mladek, linux-kernel, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Nick Desaulniers

On Tue, Jan 11, 2022 at 12:12:46AM +0200, Sakari Ailus wrote:
> On Mon, Jan 10, 2022 at 10:50:49PM +0200, Andy Shevchenko wrote:
> > The %p4cc specifier in some cases might get an unaligned pointer.
> > Due to this we need to make copy to local variable once to avoid
> > potential crashes on some architectures due to improper access.
> 
> I guess this problem exists virtually everywhere where pointers are being
> handled: the pointer could be unaligned.

True. And my patch improves the situation.
See, for example, 0f70fe605fad ("hexdump: fix for non-aligned buffers").

> Does this even address the false
> positive compiler warning?

First of all, the warning is real and legit, it's not false positive.
Second, no, this is another side of the fix.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access
  2022-01-11 10:10   ` Andy Shevchenko
@ 2022-01-11 10:54     ` Sakari Ailus
  2022-01-11 11:22       ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2022-01-11 10:54 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Petr Mladek, linux-kernel, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Nick Desaulniers

Hi Andy,

On Tue, Jan 11, 2022 at 12:10:28PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 11, 2022 at 12:12:46AM +0200, Sakari Ailus wrote:
> > On Mon, Jan 10, 2022 at 10:50:49PM +0200, Andy Shevchenko wrote:
> > > The %p4cc specifier in some cases might get an unaligned pointer.
> > > Due to this we need to make copy to local variable once to avoid
> > > potential crashes on some architectures due to improper access.
> > 
> > I guess this problem exists virtually everywhere where pointers are being
> > handled: the pointer could be unaligned.
> 
> True. And my patch improves the situation.
> See, for example, 0f70fe605fad ("hexdump: fix for non-aligned buffers").

This is different since there's no guarantee of a void pointer's alignment.

The pixelformat used for %p4cc is a pointer to u32.

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access
  2022-01-11 10:54     ` Sakari Ailus
@ 2022-01-11 11:22       ` Andy Shevchenko
  2022-01-11 14:13         ` Sakari Ailus
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2022-01-11 11:22 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Petr Mladek, linux-kernel, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Nick Desaulniers

On Tue, Jan 11, 2022 at 12:54:41PM +0200, Sakari Ailus wrote:
> On Tue, Jan 11, 2022 at 12:10:28PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 11, 2022 at 12:12:46AM +0200, Sakari Ailus wrote:
> > > On Mon, Jan 10, 2022 at 10:50:49PM +0200, Andy Shevchenko wrote:
> > > > The %p4cc specifier in some cases might get an unaligned pointer.
> > > > Due to this we need to make copy to local variable once to avoid
> > > > potential crashes on some architectures due to improper access.
> > > 
> > > I guess this problem exists virtually everywhere where pointers are being
> > > handled: the pointer could be unaligned.
> > 
> > True. And my patch improves the situation.
> > See, for example, 0f70fe605fad ("hexdump: fix for non-aligned buffers").
> 
> This is different since there's no guarantee of a void pointer's alignment.
> 
> The pixelformat used for %p4cc is a pointer to u32.

Oh, look at the %p, compiler doesn't know about the %p extensions and caller
may supply whatever they want, i.e. %p may take any address that can be kept
in void *. Actual argument _is_ void *. What you put there as u32 is just
personal expectation, and not the reality.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access
  2022-01-11 11:22       ` Andy Shevchenko
@ 2022-01-11 14:13         ` Sakari Ailus
  2022-01-11 14:18           ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2022-01-11 14:13 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Petr Mladek, linux-kernel, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Nick Desaulniers

Hi Andy,

On Tue, Jan 11, 2022 at 01:22:29PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 11, 2022 at 12:54:41PM +0200, Sakari Ailus wrote:
> > On Tue, Jan 11, 2022 at 12:10:28PM +0200, Andy Shevchenko wrote:
> > > On Tue, Jan 11, 2022 at 12:12:46AM +0200, Sakari Ailus wrote:
> > > > On Mon, Jan 10, 2022 at 10:50:49PM +0200, Andy Shevchenko wrote:
> > > > > The %p4cc specifier in some cases might get an unaligned pointer.
> > > > > Due to this we need to make copy to local variable once to avoid
> > > > > potential crashes on some architectures due to improper access.
> > > > 
> > > > I guess this problem exists virtually everywhere where pointers are being
> > > > handled: the pointer could be unaligned.
> > > 
> > > True. And my patch improves the situation.
> > > See, for example, 0f70fe605fad ("hexdump: fix for non-aligned buffers").
> > 
> > This is different since there's no guarantee of a void pointer's alignment.
> > 
> > The pixelformat used for %p4cc is a pointer to u32.
> 
> Oh, look at the %p, compiler doesn't know about the %p extensions and caller
> may supply whatever they want, i.e. %p may take any address that can be kept
> in void *. Actual argument _is_ void *. What you put there as u32 is just
> personal expectation, and not the reality.

If you assume this, you should add get_unaligned() calls in all places
where you're casting a type to another with higher alignment requirements.

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access
  2022-01-11 14:13         ` Sakari Ailus
@ 2022-01-11 14:18           ` Andy Shevchenko
  0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2022-01-11 14:18 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Petr Mladek, linux-kernel, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Nick Desaulniers

On Tue, Jan 11, 2022 at 04:13:37PM +0200, Sakari Ailus wrote:
> On Tue, Jan 11, 2022 at 01:22:29PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 11, 2022 at 12:54:41PM +0200, Sakari Ailus wrote:
> > > On Tue, Jan 11, 2022 at 12:10:28PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Jan 11, 2022 at 12:12:46AM +0200, Sakari Ailus wrote:
> > > > > On Mon, Jan 10, 2022 at 10:50:49PM +0200, Andy Shevchenko wrote:
> > > > > > The %p4cc specifier in some cases might get an unaligned pointer.
> > > > > > Due to this we need to make copy to local variable once to avoid
> > > > > > potential crashes on some architectures due to improper access.
> > > > > 
> > > > > I guess this problem exists virtually everywhere where pointers are being
> > > > > handled: the pointer could be unaligned.
> > > > 
> > > > True. And my patch improves the situation.
> > > > See, for example, 0f70fe605fad ("hexdump: fix for non-aligned buffers").
> > > 
> > > This is different since there's no guarantee of a void pointer's alignment.
> > > 
> > > The pixelformat used for %p4cc is a pointer to u32.
> > 
> > Oh, look at the %p, compiler doesn't know about the %p extensions and caller
> > may supply whatever they want, i.e. %p may take any address that can be kept
> > in void *. Actual argument _is_ void *. What you put there as u32 is just
> > personal expectation, and not the reality.
> 
> If you assume this, you should add get_unaligned() calls in all places
> where you're casting a type to another with higher alignment requirements.

Those places so far were not subject of any reports, but ideally yes, you are
right (in a scope of %p extensions, otherwise it's silly).

I will leave this to the VSPRINTF / PRINTK maintainers to proceed. Personally
I found this patch useful since we don't know what other user will do unaligned
call to it.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access
  2022-01-10 20:50 [PATCH v1 1/1] vsprintf: Fix potential unaligned access Andy Shevchenko
  2022-01-10 22:12 ` Sakari Ailus
@ 2022-01-11 14:57 ` Petr Mladek
  2022-01-11 15:09   ` Andy Shevchenko
  1 sibling, 1 reply; 9+ messages in thread
From: Petr Mladek @ 2022-01-11 14:57 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-kernel, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Nick Desaulniers, Sakari Ailus

On Mon 2022-01-10 22:50:49, Andy Shevchenko wrote:
> The %p4cc specifier in some cases might get an unaligned pointer.
> Due to this we need to make copy to local variable once to avoid
> potential crashes on some architectures due to improper access.
> 
> Fixes: af612e43de6d ("lib/vsprintf: Add support for printing V4L2 and DRM fourccs")
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks good to me:

Reviewed-by: Petr Mladek <pmladek@suse.com>

I have already sent pull request for 5.17. Could this wait
for 5.18 or would you prefer to get it into 5.17, please?

My understanding of Sakari's reply is that the current callers
provide aligned pointers. In that case it would not be urgent.
But I might have gotten it wrong.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access
  2022-01-11 14:57 ` Petr Mladek
@ 2022-01-11 15:09   ` Andy Shevchenko
  0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2022-01-11 15:09 UTC (permalink / raw)
  To: Petr Mladek
  Cc: linux-kernel, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, Nick Desaulniers, Sakari Ailus

On Tue, Jan 11, 2022 at 03:57:36PM +0100, Petr Mladek wrote:
> On Mon 2022-01-10 22:50:49, Andy Shevchenko wrote:
> > The %p4cc specifier in some cases might get an unaligned pointer.
> > Due to this we need to make copy to local variable once to avoid
> > potential crashes on some architectures due to improper access.
> > 
> > Fixes: af612e43de6d ("lib/vsprintf: Add support for printing V4L2 and DRM fourccs")
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Looks good to me:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> I have already sent pull request for 5.17. Could this wait
> for 5.18 or would you prefer to get it into 5.17, please?

It's not so critical.
But would be nice to have.

> My understanding of Sakari's reply is that the current callers
> provide aligned pointers. In that case it would not be urgent.
> But I might have gotten it wrong.

Not really. There are potential unaligned callers, but Sakari
has another patch that fixes that on the (one of *) caller side.

*) I dunno how many other callers (probably none) in tree and
   how many potentially can be with similar issue.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-01-11 15:10 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-10 20:50 [PATCH v1 1/1] vsprintf: Fix potential unaligned access Andy Shevchenko
2022-01-10 22:12 ` Sakari Ailus
2022-01-11 10:10   ` Andy Shevchenko
2022-01-11 10:54     ` Sakari Ailus
2022-01-11 11:22       ` Andy Shevchenko
2022-01-11 14:13         ` Sakari Ailus
2022-01-11 14:18           ` Andy Shevchenko
2022-01-11 14:57 ` Petr Mladek
2022-01-11 15:09   ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).