vsprintf: Fix potential unaligned access

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 <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 lib/vsprintf.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

lib/vsprintf.c +7 -5

index c130dcaca5e2..b02f01366acb 100644
@@ -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';
 

Quoted 3 lines · 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.

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?

Quoted 7 lines · Sakari Ailus

Quoted 3 lines · 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.

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").

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Sakari Ailus

Quoted 3 lines · 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.

I guess this problem exists virtually everywhere where pointers are being
handled: the pointer could be unaligned.

Quoted 2 lines

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.

Quoted 13 lines · Sakari Ailus

Quoted 11 lines · Andy Shevchenko

Quoted 7 lines · Sakari Ailus

Quoted 3 lines · 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.

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").

Quoted 3 lines

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.

Quoted 18 lines · Andy Shevchenko

Quoted 16 lines · Sakari Ailus

Quoted 11 lines · Andy Shevchenko

Quoted 7 lines · Sakari Ailus

Quoted 3 lines · 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.

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.

Quoted 4 lines

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.

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 <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Quoted 7 lines · 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 <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Looks good to me:

Reviewed-by Petr Mladek <[email protected]>

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.