[PATCH v1 1/1] vsprintf: Fix potential unaligned access

[PATCH v1 1/1] vsprintf: Fix potential unaligned access

[Andy Shevchenko]

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

Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access

[Sakari Ailus]

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

Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access

[Andy Shevchenko]

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

Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access

[Sakari Ailus]

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

Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access

[Andy Shevchenko]

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

Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access

[Sakari Ailus]

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

Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access

[Andy Shevchenko]

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

Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access

[Petr Mladek]

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

Re: [PATCH v1 1/1] vsprintf: Fix potential unaligned access

[Andy Shevchenko]

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