[PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages

[PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages

[Joe Perches]

Printing kernel pointers via %pK has a minor defect when
kptr_restrict is set to 2:  the pointer may be emitted
as "pK-error" instead of all 0's when in an interrupt.

Fix this defect, neaten the code, and correct the sysctl
documentation.

Add missing documentation for 2 other uses: %pNF and %pV.

Trivially reduces vsprintf.o object size:

$ size lib/vsprintf.o*
   text	   data	    bss	    dec	    hex	filename
  14536	      6	      0	  14542	   38ce	lib/vsprintf.o.new
  14568	      6	      0	  14574	   38ee	lib/vsprintf.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/sysctl/kernel.txt | 17 +++++++++--------
 lib/vsprintf.c                  | 38 ++++++++++++++++++++++++--------------
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 9d4c1d1..c17d5ca 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -289,14 +289,15 @@ Default value is "/sbin/hotplug".
 
 kptr_restrict:
 
-This toggle indicates whether restrictions are placed on
-exposing kernel addresses via /proc and other interfaces.  When
-kptr_restrict is set to (0), there are no restrictions.  When
-kptr_restrict is set to (1), the default, kernel pointers
-printed using the %pK format specifier will be replaced with 0's
-unless the user has CAP_SYSLOG.  When kptr_restrict is set to
-(2), kernel pointers printed using %pK will be replaced with 0's
-regardless of privileges.
+This toggle indicates whether restrictions are placed on exposing kernel
+addresses via /proc and other interfaces.
+
+When kptr_restrict is set to (0), the default, there are no restrictions.
+When kptr_restrict is set to (1), kernel pointers printed using the %pK
+format specifier will be replaced with 0's unless the user has CAP_SYSLOG
+and effective user and group ids are equal to the real ids.
+When kptr_restrict is set to (2), kernel pointers printed using %pK will
+be replaced with 0's regardless of privileges.
 
 ==============================================================
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..ce55f52 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1301,21 +1301,28 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			va_end(va);
 			return buf;
 		}
-	case 'K':
-		/*
-		 * %pK cannot be used in IRQ context because its test
-		 * for CAP_SYSLOG would be meaningless.
-		 */
-		if (kptr_restrict && (in_irq() || in_serving_softirq() ||
-				      in_nmi())) {
-			if (spec.field_width == -1)
-				spec.field_width = default_width;
-			return string(buf, end, "pK-error", spec);
-		}
-		if (!((kptr_restrict == 0) ||
-		      (kptr_restrict == 1 &&
-		       has_capability_noaudit(current, CAP_SYSLOG))))
+	case 'K':		/* see: Documentation/sysctl/kernel.txt */
+		switch (kptr_restrict) {
+		case 0:			/* None (default) */
+			break;
+		case 1:			/* Restricted */
+			if (in_irq() || in_serving_softirq() || in_nmi()) {
+				/*
+				 * This cannot be used in IRQ context because
+				 * the test for CAP_SYSLOG would be meaningless
+				 */
+				if (spec.field_width == -1)
+					spec.field_width = default_width;
+				return string(buf, end, "pK-error", spec);
+			}
+			if (!has_capability_noaudit(current, CAP_SYSLOG))
+				ptr = NULL;
+			break;
+		case 2:			/* Never - Always emit 0 */
+		default:
 			ptr = NULL;
+			break;
+		}
 		break;
 	case 'N':
 		switch (fmt[1]) {
@@ -1574,6 +1581,9 @@ qualifier:
  * %piS depending on sa_family of 'struct sockaddr *' print IPv4/IPv6 address
  * %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
  *   case.
+ * %pV recurse and output a struct va_format (const char *fmt, va_list *)
+ * %pK output a kernel address or 0 depending on sysctl kptr_restrict
+ * %NF output a netdev_features_t
  * %*ph[CDN] a variable-length hex string with a separator (supports up to 64
  *           bytes of the input)
  * %n is ignored
-- 
1.8.1.2.459.gbcd45b4.dirty

[PATCH 2/2] vsprintf: Just some neatening

[Joe Perches]

Remove a few coding style nits:

o Leading spaces for tabs
o case indentation depth
o else and brace locations
o logical continuation placement

Several checkpatch bleats still exist,
mostly not worth changing.

Signed-off-by: Joe Perches <joe@perches.com>
---
 lib/vsprintf.c | 83 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 42 insertions(+), 41 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index ce55f52..dbc945b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -266,10 +266,10 @@ void put_dec_full4(char *buf, unsigned q)
 static
 unsigned put_dec_helper4(char *buf, unsigned x)
 {
-        uint32_t q = (x * (uint64_t)0x346DC5D7) >> 43;
+	uint32_t q = (x * (uint64_t)0x346DC5D7) >> 43;
 
-        put_dec_full4(buf, x - q * 10000);
-        return q;
+	put_dec_full4(buf, x - q * 10000);
+	return q;
 }
 
 /* Based on code by Douglas W. Jones found at
@@ -561,11 +561,11 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 	int i, n;
 
 	switch (fmt[1]) {
-		case '2': case '3': case '4':
-			depth = fmt[1] - '0';
-			break;
-		default:
-			depth = 1;
+	case '2': case '3': case '4':
+		depth = fmt[1] - '0';
+		break;
+	default:
+		depth = 1;
 	}
 
 	rcu_read_lock();
@@ -939,11 +939,11 @@ char *ip6_compressed_string(char *p, const char *addr)
 			else
 				*p++ = hex_asc_lo(hi);
 			p = hex_byte_pack(p, lo);
-		}
-		else if (lo > 0x0f)
+		} else if (lo > 0x0f) {
 			p = hex_byte_pack(p, lo);
-		else
+		} else {
 			*p++ = hex_asc_lo(lo);
+		}
 		needcolon = true;
 	}
 
@@ -1268,10 +1268,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 					 * 6:   000102...0f
 					 */
 		switch (fmt[1]) {
-		case '6':
-			return ip6_addr_string(buf, end, ptr, spec, fmt);
-		case '4':
-			return ip4_addr_string(buf, end, ptr, spec, fmt);
 		case 'S': {
 			const union {
 				struct sockaddr		raw;
@@ -1281,26 +1277,33 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 
 			switch (sa->raw.sa_family) {
 			case AF_INET:
-				return ip4_addr_string_sa(buf, end, &sa->v4, spec, fmt);
+				return ip4_addr_string_sa(buf, end, &sa->v4,
+							  spec, fmt);
 			case AF_INET6:
-				return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
+				return ip6_addr_string_sa(buf, end, &sa->v6,
+							  spec, fmt);
 			default:
-				return string(buf, end, "(invalid address)", spec);
-			}}
+				return string(buf, end, "(invalid address)",
+					      spec);
+			}
+		}
+		case '6':
+			return ip6_addr_string(buf, end, ptr, spec, fmt);
+		case '4':
+			return ip4_addr_string(buf, end, ptr, spec, fmt);
 		}
 		break;
 	case 'U':
 		return uuid_string(buf, end, ptr, spec, fmt);
-	case 'V':
-		{
-			va_list va;
+	case 'V': {
+		va_list va;
 
-			va_copy(va, *((struct va_format *)ptr)->va);
-			buf += vsnprintf(buf, end > buf ? end - buf : 0,
-					 ((struct va_format *)ptr)->fmt, va);
-			va_end(va);
-			return buf;
-		}
+		va_copy(va, *((struct va_format *)ptr)->va);
+		buf += vsnprintf(buf, end > buf ? end - buf : 0,
+				 ((struct va_format *)ptr)->fmt, va);
+		va_end(va);
+		return buf;
+	}
 	case 'K':		/* see: Documentation/sysctl/kernel.txt */
 		switch (kptr_restrict) {
 		case 0:			/* None (default) */
@@ -1400,7 +1403,7 @@ int format_decode(const char *fmt, struct printf_spec *spec)
 	/* By default */
 	spec->type = FORMAT_TYPE_NONE;
 
-	for (; *fmt ; ++fmt) {
+	for (; *fmt; ++fmt) {
 		if (*fmt == '%')
 			break;
 	}
@@ -1433,9 +1436,9 @@ int format_decode(const char *fmt, struct printf_spec *spec)
 	/* get field width */
 	spec->field_width = -1;
 
-	if (isdigit(*fmt))
+	if (isdigit(*fmt)) {
 		spec->field_width = skip_atoi(&fmt);
-	else if (*fmt == '*') {
+	} else if (*fmt == '*') {
 		/* it's the next argument */
 		spec->type = FORMAT_TYPE_WIDTH;
 		return ++fmt - start;
@@ -1521,9 +1524,9 @@ qualifier:
 		return fmt - start;
 	}
 
-	if (spec->qualifier == 'L')
+	if (spec->qualifier == 'L') {
 		spec->type = FORMAT_TYPE_LONG_LONG;
-	else if (spec->qualifier == 'l') {
+	} else if (spec->qualifier == 'l') {
 		if (spec->flags & SIGN)
 			spec->type = FORMAT_TYPE_LONG;
 		else
@@ -1654,7 +1657,6 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 					if (str < end)
 						*str = ' ';
 					++str;
-
 				}
 			}
 			c = (unsigned char) va_arg(args, int);
@@ -1760,7 +1762,6 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 
 	/* the trailing null byte doesn't count towards the total */
 	return str-buf;
-
 }
 EXPORT_SYMBOL(vsnprintf);
 
@@ -1966,7 +1967,7 @@ do {									\
 			size_t len;
 
 			if ((unsigned long)save_str > (unsigned long)-PAGE_SIZE
-					|| (unsigned long)save_str < PAGE_SIZE)
+			    || (unsigned long)save_str < PAGE_SIZE)
 				save_str = "(null)";
 			len = strlen(save_str) + 1;
 			if (str + len < end)
@@ -2392,11 +2393,11 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		if (is_sign && digit == '-')
 			digit = *(str + 1);
 
-		if (!digit
-		    || (base == 16 && !isxdigit(digit))
-		    || (base == 10 && !isdigit(digit))
-		    || (base == 8 && (!isdigit(digit) || digit > '7'))
-		    || (base == 0 && !isdigit(digit)))
+		if (!digit ||
+		    (base == 16 && !isxdigit(digit)) ||
+		    (base == 10 && !isdigit(digit)) ||
+		    (base == 8 && (!isdigit(digit) || digit > '7')) ||
+		    (base == 0 && !isdigit(digit)))
 			break;
 
 		if (is_sign)
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages

[Ryan Mallon]

On 11/10/13 16:25, Joe Perches wrote:
> Printing kernel pointers via %pK has a minor defect when
> kptr_restrict is set to 2:  the pointer may be emitted
> as "pK-error" instead of all 0's when in an interrupt.

NAK. This is not a defect, as I explained earlier. It is really a defect
that it _doesn't_ print 'pK-error' in all cases. 'pK-error' is for
finding kernel bugs.

If a user sets kptr_restrict to 0 (or 2 with this patch), then pointer
values printed from interrupt handlers will appear as normal (all 0's in
kptr_restrict=2 case), so the user will not notice that the kernel code
is actually buggy. If they ever set kptr_restrict to 1 then they will
start seeing the 'pK-error' value. Since the default kptr_restrict
setting is now 0, it is less likely that users/developers will see the
'pK-error' message for buggy code.

~Ryan

> 
> Fix this defect, neaten the code, and correct the sysctl
> documentation.
> 
> Add missing documentation for 2 other uses: %pNF and %pV.
> 
> Trivially reduces vsprintf.o object size:
> 
> $ size lib/vsprintf.o*
>    text	   data	    bss	    dec	    hex	filename
>   14536	      6	      0	  14542	   38ce	lib/vsprintf.o.new
>   14568	      6	      0	  14574	   38ee	lib/vsprintf.o.old
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  Documentation/sysctl/kernel.txt | 17 +++++++++--------
>  lib/vsprintf.c                  | 38 ++++++++++++++++++++++++--------------
>  2 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 9d4c1d1..c17d5ca 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -289,14 +289,15 @@ Default value is "/sbin/hotplug".
>  
>  kptr_restrict:
>  
> -This toggle indicates whether restrictions are placed on
> -exposing kernel addresses via /proc and other interfaces.  When
> -kptr_restrict is set to (0), there are no restrictions.  When
> -kptr_restrict is set to (1), the default, kernel pointers
> -printed using the %pK format specifier will be replaced with 0's
> -unless the user has CAP_SYSLOG.  When kptr_restrict is set to
> -(2), kernel pointers printed using %pK will be replaced with 0's
> -regardless of privileges.
> +This toggle indicates whether restrictions are placed on exposing kernel
> +addresses via /proc and other interfaces.
> +
> +When kptr_restrict is set to (0), the default, there are no restrictions.
> +When kptr_restrict is set to (1), kernel pointers printed using the %pK
> +format specifier will be replaced with 0's unless the user has CAP_SYSLOG
> +and effective user and group ids are equal to the real ids.
> +When kptr_restrict is set to (2), kernel pointers printed using %pK will
> +be replaced with 0's regardless of privileges.
>  
>  ==============================================================
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 26559bd..ce55f52 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1301,21 +1301,28 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  			va_end(va);
>  			return buf;
>  		}
> -	case 'K':
> -		/*
> -		 * %pK cannot be used in IRQ context because its test
> -		 * for CAP_SYSLOG would be meaningless.
> -		 */
> -		if (kptr_restrict && (in_irq() || in_serving_softirq() ||
> -				      in_nmi())) {
> -			if (spec.field_width == -1)
> -				spec.field_width = default_width;
> -			return string(buf, end, "pK-error", spec);
> -		}
> -		if (!((kptr_restrict == 0) ||
> -		      (kptr_restrict == 1 &&
> -		       has_capability_noaudit(current, CAP_SYSLOG))))
> +	case 'K':		/* see: Documentation/sysctl/kernel.txt */
> +		switch (kptr_restrict) {
> +		case 0:			/* None (default) */
> +			break;
> +		case 1:			/* Restricted */
> +			if (in_irq() || in_serving_softirq() || in_nmi()) {
> +				/*
> +				 * This cannot be used in IRQ context because
> +				 * the test for CAP_SYSLOG would be meaningless
> +				 */
> +				if (spec.field_width == -1)
> +					spec.field_width = default_width;
> +				return string(buf, end, "pK-error", spec);
> +			}
> +			if (!has_capability_noaudit(current, CAP_SYSLOG))
> +				ptr = NULL;
> +			break;
> +		case 2:			/* Never - Always emit 0 */
> +		default:
>  			ptr = NULL;
> +			break;
> +		}
>  		break;
>  	case 'N':
>  		switch (fmt[1]) {
> @@ -1574,6 +1581,9 @@ qualifier:
>   * %piS depending on sa_family of 'struct sockaddr *' print IPv4/IPv6 address
>   * %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
>   *   case.
> + * %pV recurse and output a struct va_format (const char *fmt, va_list *)
> + * %pK output a kernel address or 0 depending on sysctl kptr_restrict
> + * %NF output a netdev_features_t
>   * %*ph[CDN] a variable-length hex string with a separator (supports up to 64
>   *           bytes of the input)
>   * %n is ignored
> 

Re: [PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages

[Joe Perches]

On Fri, 2013-10-11 at 16:31 +1100, Ryan Mallon wrote:
> On 11/10/13 16:25, Joe Perches wrote:
> > Printing kernel pointers via %pK has a minor defect when
> > kptr_restrict is set to 2:  the pointer may be emitted
> > as "pK-error" instead of all 0's when in an interrupt.
> 
> NAK. This is not a defect, as I explained earlier. It is really a defect
> that it _doesn't_ print 'pK-error' in all cases. 'pK-error' is for
> finding kernel bugs.

Not my understanding.

There is no bug to find when emitting a pointer via %pK.

The only issue is that has_capability_noaudit can not
be called from an interrupt.

Re: [PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages

[Ryan Mallon]

On 11/10/13 16:38, Joe Perches wrote:
> On Fri, 2013-10-11 at 16:31 +1100, Ryan Mallon wrote:
>> On 11/10/13 16:25, Joe Perches wrote:
>>> Printing kernel pointers via %pK has a minor defect when
>>> kptr_restrict is set to 2:  the pointer may be emitted
>>> as "pK-error" instead of all 0's when in an interrupt.
>>
>> NAK. This is not a defect, as I explained earlier. It is really a defect
>> that it _doesn't_ print 'pK-error' in all cases. 'pK-error' is for
>> finding kernel bugs.
> 
> Not my understanding.
> 
> There is no bug to find when emitting a pointer via %pK.

Yes there is. %pK is not valid from interrupt context, because the
current way it is checked is by checking CAP_SYSLOG. It is irrelevant
that this check is only done when kptr_restrict=1, using %pK from
interrupt handlers is still wrong. We want to notify developers of any
wrong cases.

> 
> The only issue is that has_capability_noaudit can not
> be called from an interrupt.

Right. So any code that uses %pK from interrupt context would be, by
definition, broken. That is what the check is looking for.

It doesn't matter what the value of kptr_restrict happens to be, the
code is still broken. So, with your patch, values 0 and 2 of
kptr_restrict will print a seemingly correct value, but when
kptr_restrict is 1 then it will print 'pK-error'.

That is confusing to users: file prints correct pointer values when
kptr_restrict=0; file prints 'pK-error' when kptr_restrict=1, even
though I am root. Also, because the default is now kptr_restrict=0, less
people will see the 'pK-error' value, so buggy code may go over-looked.

It should print 'pK-error' in all cases, so that any bugs where %pK is
being used from interrupt content are identified regardless of the
setting of kptr_restrict.

Anyway, with the approach that Eric and George suggested, this would
become a non-issue. So probably just best to leave the code as is.

~Ryan

Re: [PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages

[Joe Perches]

On Fri, 2013-10-11 at 16:49 +1100, Ryan Mallon wrote:

> It doesn't matter what the value of kptr_restrict happens to be, the
> code is still broken. So, with your patch, values 0 and 2 of
> kptr_restrict will print a seemingly correct value, but when
> kptr_restrict is 1 then it will print 'pK-error'.

I think it's _wrong_ to break existing code.

And no, 0 and 2 will print "correct" values.

Only the relatively uncommon 1 use case is broken.
kptr_restrict is simply broken relative to interrupts.

%pK is used outside of seq and goes to dmesg too btw.
You can't simply check the process permissions at open.

> It should print 'pK-error' in all cases, so that any bugs where %pK is
> being used from interrupt content are identified regardless of the
> setting of kptr_restrict.
> 
> Anyway, with the approach that Eric and George suggested, this would
> become a non-issue. So probably just best to leave the code as is.

Or do as I did and shrink it and make it clearer
until your use cases might be implemented.