[PATCH] vsprintf: kptr_restrict is okay in IRQ when 2

[PATCH] vsprintf: kptr_restrict is okay in IRQ when 2

[Jason A. Donenfeld]

The kptr_restrict flag, when set to 1, only prints the kernel
address when the user has CAP_SYSLOG. When it is set to 2, the
kernel address is always printed as zero. When set to 1, this
needs to check whether or not we're in IRQ. However, when set to
2, this check is unneccessary, and produces confusing results
in dmesg. Thus, only make sure we're not in IRQ when mode 1 is
used, but not mode 2.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 lib/vsprintf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 80d8ce5..fab875f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1609,8 +1609,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		 * %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 (kptr_restrict == 1 && (in_irq() || in_serving_softirq() ||
+				           in_nmi())) {
 			if (spec.field_width == -1)
 				spec.field_width = default_width;
 			return string(buf, end, "pK-error", spec);
-- 
2.7.0

Re: [PATCH] vsprintf: kptr_restrict is okay in IRQ when 2

[Kees Cook]

On Fri, Feb 5, 2016 at 5:45 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> The kptr_restrict flag, when set to 1, only prints the kernel
> address when the user has CAP_SYSLOG. When it is set to 2, the
> kernel address is always printed as zero. When set to 1, this
> needs to check whether or not we're in IRQ. However, when set to
> 2, this check is unneccessary, and produces confusing results
> in dmesg. Thus, only make sure we're not in IRQ when mode 1 is
> used, but not mode 2.

Cool, nice fix.

>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  lib/vsprintf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 80d8ce5..fab875f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1609,8 +1609,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>                  * %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 (kptr_restrict == 1 && (in_irq() || in_serving_softirq() ||
> +                                          in_nmi())) {

Instead of doing a double-check of kptr_restrict, how about moving
this logic down into the "case 1" test? I think that would be more
readable in the end.

-Kees

>                         if (spec.field_width == -1)
>                                 spec.field_width = default_width;
>                         return string(buf, end, "pK-error", spec);
> --
> 2.7.0
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] vsprintf: kptr_restrict is okay in IRQ when 2

[Jason A. Donenfeld]

On Fri, Feb 5, 2016 at 9:46 PM, Kees Cook <keescook@chromium.org> wrote:
>
> Instead of doing a double-check of kptr_restrict, how about moving
> this logic down into the "case 1" test? I think that would be more
> readable in the end.

Good thinking. Will roll v2.

[PATCH v2] vsprintf: kptr_restrict is okay in IRQ when 2

[Jason A. Donenfeld]

The kptr_restrict flag, when set to 1, only prints the kernel
address when the user has CAP_SYSLOG. When it is set to 2, the
kernel address is always printed as zero. When set to 1, this
needs to check whether or not we're in IRQ. However, when set to
2, this check is unneccessary, and produces confusing results
in dmesg. Thus, only make sure we're not in IRQ when mode 1 is
used, but not mode 2.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 lib/vsprintf.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 80d8ce5..ee1e24e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1605,22 +1605,23 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			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);
-		}
-
 		switch (kptr_restrict) {
 		case 0:
 			/* Always print %pK values */
 			break;
 		case 1: {
+			const struct cred *cred;
+
+			/*
+			 * kptr_restrict==2 cannot be used in IRQ context because
+			 * its test for CAP_SYSLOG would be meaningless.
+			 */
+			if (in_irq() || in_serving_softirq() || in_nmi()) {
+				if (spec.field_width == -1)
+					spec.field_width = default_width;
+				return string(buf, end, "pK-error", spec);
+			}
+
 			/*
 			 * Only print the real pointer value if the current
 			 * process has CAP_SYSLOG and is running with the
@@ -1630,8 +1631,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			 * leak pointer values if a binary opens a file using
 			 * %pK and then elevates privileges before reading it.
 			 */
-			const struct cred *cred = current_cred();
-
+			cred = current_cred();
 			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
 			    !uid_eq(cred->euid, cred->uid) ||
 			    !gid_eq(cred->egid, cred->gid))
-- 
2.7.0

Re: [PATCH v2] vsprintf: kptr_restrict is okay in IRQ when 2

[Kees Cook]

On Fri, Feb 5, 2016 at 2:03 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> The kptr_restrict flag, when set to 1, only prints the kernel
> address when the user has CAP_SYSLOG. When it is set to 2, the
> kernel address is always printed as zero. When set to 1, this
> needs to check whether or not we're in IRQ. However, when set to
> 2, this check is unneccessary, and produces confusing results
> in dmesg. Thus, only make sure we're not in IRQ when mode 1 is
> used, but not mode 2.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Thanks!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  lib/vsprintf.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 80d8ce5..ee1e24e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1605,22 +1605,23 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>                         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);
> -               }
> -
>                 switch (kptr_restrict) {
>                 case 0:
>                         /* Always print %pK values */
>                         break;
>                 case 1: {
> +                       const struct cred *cred;
> +
> +                       /*
> +                        * kptr_restrict==2 cannot be used in IRQ context because
> +                        * its test for CAP_SYSLOG would be meaningless.
> +                        */
> +                       if (in_irq() || in_serving_softirq() || in_nmi()) {
> +                               if (spec.field_width == -1)
> +                                       spec.field_width = default_width;
> +                               return string(buf, end, "pK-error", spec);
> +                       }
> +
>                         /*
>                          * Only print the real pointer value if the current
>                          * process has CAP_SYSLOG and is running with the
> @@ -1630,8 +1631,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>                          * leak pointer values if a binary opens a file using
>                          * %pK and then elevates privileges before reading it.
>                          */
> -                       const struct cred *cred = current_cred();
> -
> +                       cred = current_cred();
>                         if (!has_capability_noaudit(current, CAP_SYSLOG) ||
>                             !uid_eq(cred->euid, cred->uid) ||
>                             !gid_eq(cred->egid, cred->gid))
> --
> 2.7.0
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2] vsprintf: kptr_restrict is okay in IRQ when 2

[Rasmus Villemoes]

On Fri, Feb 05 2016, Kees Cook <keescook@chromium.org> wrote:

>>                 switch (kptr_restrict) {
>>                 case 0:
>>                         /* Always print %pK values */
>>                         break;
>>                 case 1: {
>> +                       const struct cred *cred;
>> +
>> +                       /*
>> +                        * kptr_restrict==2 cannot be used in IRQ context because
                                             ^

should be 1, right?

Rasmus

[PATCH v3] vsprintf: kptr_restrict is okay in IRQ when 2

[Jason A. Donenfeld]

The kptr_restrict flag, when set to 1, only prints the kernel
address when the user has CAP_SYSLOG. When it is set to 2, the
kernel address is always printed as zero. When set to 1, this
needs to check whether or not we're in IRQ. However, when set to
2, this check is unneccessary, and produces confusing results
in dmesg. Thus, only make sure we're not in IRQ when mode 1 is
used, but not mode 2.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 lib/vsprintf.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 80d8ce5..37c7068 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1605,22 +1605,23 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			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);
-		}
-
 		switch (kptr_restrict) {
 		case 0:
 			/* Always print %pK values */
 			break;
 		case 1: {
+			const struct cred *cred;
+
+			/*
+			 * kptr_restrict==1 cannot be used in IRQ context because
+			 * its test for CAP_SYSLOG would be meaningless.
+			 */
+			if (in_irq() || in_serving_softirq() || in_nmi()) {
+				if (spec.field_width == -1)
+					spec.field_width = default_width;
+				return string(buf, end, "pK-error", spec);
+			}
+
 			/*
 			 * Only print the real pointer value if the current
 			 * process has CAP_SYSLOG and is running with the
@@ -1630,8 +1631,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			 * leak pointer values if a binary opens a file using
 			 * %pK and then elevates privileges before reading it.
 			 */
-			const struct cred *cred = current_cred();
-
+			cred = current_cred();
 			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
 			    !uid_eq(cred->euid, cred->uid) ||
 			    !gid_eq(cred->egid, cred->gid))
-- 
2.7.0