Re: [PATCH] MAINTAINERS: EP93XX: Update maintainership
On 22/05/17 23:30, Alexander Sverdlin wrote: > As agreed with Ryan, change the maintainership. Thanks for taking this over Alexander. Acked-by: Ryan Mallon <rmal...@gmail.com> > Signed-off-by: Alexander Sverdlin <alexander.sverd...@gmail.com> > --- > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9e984645c4b0..5a248d0f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1172,7 +1172,7 @@ N: clps711x > > ARM/CIRRUS LOGIC EP93XX ARM ARCHITECTURE > M: Hartley Sweeten <hswee...@visionengravers.com> > -M: Ryan Mallon <rmal...@gmail.com> > +M: Alexander Sverdlin <alexander.sverd...@gmail.com> > L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) > S: Maintained > F: arch/arm/mach-ep93xx/ >
Re: [PATCH] MAINTAINERS: EP93XX: Update maintainership
On 22/05/17 23:30, Alexander Sverdlin wrote: > As agreed with Ryan, change the maintainership. Thanks for taking this over Alexander. Acked-by: Ryan Mallon > Signed-off-by: Alexander Sverdlin > --- > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9e984645c4b0..5a248d0f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1172,7 +1172,7 @@ N: clps711x > > ARM/CIRRUS LOGIC EP93XX ARM ARCHITECTURE > M: Hartley Sweeten > -M: Ryan Mallon > +M: Alexander Sverdlin > L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) > S: Maintained > F: arch/arm/mach-ep93xx/ >
Re: [PATCH v2] serial: serial_core.c: printk replacement
On 29/08/14 00:15, Sudip Mukherjee wrote:
> printk replaced with corresponding pr_err, dev_alert, dev_notice and pr_info.
> fixed two broken user-visible strings used by the corresponding printk
>
> Signed-off-by: Sudip Mukherjee
> ---
>
> In the first patch i sent Greg suggested to use dev_err instead of pr_err ,
> but as later Jiri pointed out in my later patches that uport->dev is having a
> null check so can not be used , tty_dev is also having a put_device , so back
> to pr_err.
>
> drivers/tty/serial/serial_core.c | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> @@ -1975,12 +1976,11 @@ int uart_suspend_port(struct uart_driver *drv, struct
> uart_port *uport)
> for (tries = 3; !ops->tx_empty(uport) && tries; tries--)
> msleep(10);
> if (!tries)
> - printk(KERN_ERR "%s%s%s%d: Unable to drain "
> - "transmitter\n",
> -uport->dev ? dev_name(uport->dev) : "",
> -uport->dev ? ": " : "",
> -drv->dev_name,
> -drv->tty_driver->name_base + uport->line);
> + pr_err("%s%s%s%d: Unable to drain transmitter\n",
> + uport->dev ? dev_name(uport->dev) : "",
> + uport->dev ? ": " : "",
> + drv->dev_name,
> + drv->tty_driver->name_base + uport->line);
dev_printk() prints the value of dev_name() and handles the NULL case,
so this could be changed to:
dev_err(uport->dev, "%s%d: Unable to drain transmitter\n",
drv->dev_name,
drv->tty_driver->name_base + uport->line);
It might also be possible to remove drv->dev_name since dev_printk()
also prints the driver string, but I don't know if they are equivalent
in this case.
>
> if (console_suspend_enabled || !uart_console(uport))
> ops->shutdown(uport);
> @@ -2109,7 +2109,7 @@ uart_report_port(struct uart_driver *drv, struct
> uart_port *port)
> break;
> }
>
> - printk(KERN_INFO "%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
> + pr_info("%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
> port->dev ? dev_name(port->dev) : "",
> port->dev ? ": " : "",
> drv->dev_name,
Same here.
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] serial: serial_core.c: printk replacement
On 29/08/14 00:15, Sudip Mukherjee wrote: printk replaced with corresponding pr_err, dev_alert, dev_notice and pr_info. fixed two broken user-visible strings used by the corresponding printk Signed-off-by: Sudip Mukherjee su...@vectorindia.org --- In the first patch i sent Greg suggested to use dev_err instead of pr_err , but as later Jiri pointed out in my later patches that uport-dev is having a null check so can not be used , tty_dev is also having a put_device , so back to pr_err. drivers/tty/serial/serial_core.c | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) @@ -1975,12 +1976,11 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport) for (tries = 3; !ops-tx_empty(uport) tries; tries--) msleep(10); if (!tries) - printk(KERN_ERR %s%s%s%d: Unable to drain - transmitter\n, -uport-dev ? dev_name(uport-dev) : , -uport-dev ? : : , -drv-dev_name, -drv-tty_driver-name_base + uport-line); + pr_err(%s%s%s%d: Unable to drain transmitter\n, + uport-dev ? dev_name(uport-dev) : , + uport-dev ? : : , + drv-dev_name, + drv-tty_driver-name_base + uport-line); dev_printk() prints the value of dev_name() and handles the NULL case, so this could be changed to: dev_err(uport-dev, %s%d: Unable to drain transmitter\n, drv-dev_name, drv-tty_driver-name_base + uport-line); It might also be possible to remove drv-dev_name since dev_printk() also prints the driver string, but I don't know if they are equivalent in this case. if (console_suspend_enabled || !uart_console(uport)) ops-shutdown(uport); @@ -2109,7 +2109,7 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port) break; } - printk(KERN_INFO %s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n, + pr_info(%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n, port-dev ? dev_name(port-dev) : , port-dev ? : : , drv-dev_name, Same here. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sgi-xp: Do not use BUG_ON(!spin_is_locked())
On 12/08/14 17:35, Sanjeev Sharma wrote:
> on some architecture spin_is_locked() always return false in
> uniprocessor configuration and can therefore not be used
> with BUG_ON.it would be advise to replace with
> lockdep_assert_held().
>
> Signed-off-by: Sanjeev Sharma
> ---
> drivers/misc/sgi-xp/xpc_channel.c | 6 +++---
> drivers/misc/sgi-xp/xpc_sn2.c | 2 +-
> drivers/misc/sgi-xp/xpc_uv.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/misc/sgi-xp/xpc_channel.c
> b/drivers/misc/sgi-xp/xpc_channel.c
> index 128d561..240fe5a 100644
> --- a/drivers/misc/sgi-xp/xpc_channel.c
> +++ b/drivers/misc/sgi-xp/xpc_channel.c
> @@ -28,7 +28,7 @@ xpc_process_connect(struct xpc_channel *ch, unsigned long
> *irq_flags)
> {
> enum xp_retval ret;
>
> - DBUG_ON(!spin_is_locked(>lock));
> + lockdep_assert_held(!spin_is_locked(>lock));
This is incorrect, and will break the build with CONFIG_LOCKDEP enabled.
You actually want:
lockdep_assert_held(>lock);
Same for the other instances.
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sgi-xp: Do not use BUG_ON(!spin_is_locked())
On 12/08/14 17:35, Sanjeev Sharma wrote:
on some architecture spin_is_locked() always return false in
uniprocessor configuration and can therefore not be used
with BUG_ON.it would be advise to replace with
lockdep_assert_held().
Signed-off-by: Sanjeev Sharma sanjeev_sha...@mentor.com
---
drivers/misc/sgi-xp/xpc_channel.c | 6 +++---
drivers/misc/sgi-xp/xpc_sn2.c | 2 +-
drivers/misc/sgi-xp/xpc_uv.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/sgi-xp/xpc_channel.c
b/drivers/misc/sgi-xp/xpc_channel.c
index 128d561..240fe5a 100644
--- a/drivers/misc/sgi-xp/xpc_channel.c
+++ b/drivers/misc/sgi-xp/xpc_channel.c
@@ -28,7 +28,7 @@ xpc_process_connect(struct xpc_channel *ch, unsigned long
*irq_flags)
{
enum xp_retval ret;
- DBUG_ON(!spin_is_locked(ch-lock));
+ lockdep_assert_held(!spin_is_locked(ch-lock));
This is incorrect, and will break the build with CONFIG_LOCKDEP enabled.
You actually want:
lockdep_assert_held(ch-lock);
Same for the other instances.
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] video: Use fb_sys_write rather than open-coding in drivers
On 12/02/14 19:54, Tomi Valkeinen wrote: > On 11/02/14 21:07, Ryan Mallon wrote: >> On 12/02/14 03:06, Tomi Valkeinen wrote: >> >>> On 20/09/13 10:06, Ryan Mallon wrote: >>>> Several video drivers open code the fb_write write function with code >>>> which is very similar to fb_sys_write. Replace the open code versions >>>> with calls to fb_sys_write. An fb_sync callback is added to each of >>>> the drivers. >>>> >>>> Signed-off-by: Ryan Mallon >>>> --- >>> >>> Doesn't this change the behavior so that fb_write does no longer update >>> the display, but fb_sync does? I don't think fb_sync is even meant to >>> update the display, it's meant to wait for an update to finish. Then >>> again, I'm not sure about that, all I see in fb.h is "wait for blit >>> idle, optional" >> >> >> fb_write() in fbmem.c calls ->fb_sync() after ->fb_write(), and I've set >> the fb_sync() for each of the drivers, so the behaviour should be >> unchanged for writes. >> >> The fb_sync() function is also called by fb_read() and >> fb_get_buffer_offset() (if FB_PIXMAP_SYNC flag is set). I don't know if >> that will adversely affect behaviour. > > Well, by just looking at the function names the drivers' fb_syncs call, > it sounds to me that with your patch fb_sync will update the LCD, i.e. > send data to it. Doing that in fb_read sounds totally wrong. Well, the alternative is to supply an fb_write() implementation for each driver that calls fb_sys_write(), and then updates the display. The fb_sync() additions can be removed. That would cut down the boiler-plate code, and should keep the behaviour the same. If you don't think it is worth the effort, then the patch can just be dropped. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] video: Use fb_sys_write rather than open-coding in drivers
On 12/02/14 03:06, Tomi Valkeinen wrote: > On 20/09/13 10:06, Ryan Mallon wrote: >> Several video drivers open code the fb_write write function with code >> which is very similar to fb_sys_write. Replace the open code versions >> with calls to fb_sys_write. An fb_sync callback is added to each of >> the drivers. >> >> Signed-off-by: Ryan Mallon >> --- > > Doesn't this change the behavior so that fb_write does no longer update > the display, but fb_sync does? I don't think fb_sync is even meant to > update the display, it's meant to wait for an update to finish. Then > again, I'm not sure about that, all I see in fb.h is "wait for blit > idle, optional" fb_write() in fbmem.c calls ->fb_sync() after ->fb_write(), and I've set the fb_sync() for each of the drivers, so the behaviour should be unchanged for writes. The fb_sync() function is also called by fb_read() and fb_get_buffer_offset() (if FB_PIXMAP_SYNC flag is set). I don't know if that will adversely affect behaviour. Note that I haven't actually tested this code since I don't have any of the hardware. It was just something I noticed while digging through framebuffer driver code. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] video: Use fb_sys_write rather than open-coding in drivers
On 12/02/14 03:06, Tomi Valkeinen wrote: On 20/09/13 10:06, Ryan Mallon wrote: Several video drivers open code the fb_write write function with code which is very similar to fb_sys_write. Replace the open code versions with calls to fb_sys_write. An fb_sync callback is added to each of the drivers. Signed-off-by: Ryan Mallon rmal...@gmail.com --- Doesn't this change the behavior so that fb_write does no longer update the display, but fb_sync does? I don't think fb_sync is even meant to update the display, it's meant to wait for an update to finish. Then again, I'm not sure about that, all I see in fb.h is wait for blit idle, optional fb_write() in fbmem.c calls -fb_sync() after -fb_write(), and I've set the fb_sync() for each of the drivers, so the behaviour should be unchanged for writes. The fb_sync() function is also called by fb_read() and fb_get_buffer_offset() (if FB_PIXMAP_SYNC flag is set). I don't know if that will adversely affect behaviour. Note that I haven't actually tested this code since I don't have any of the hardware. It was just something I noticed while digging through framebuffer driver code. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] video: Use fb_sys_write rather than open-coding in drivers
On 12/02/14 19:54, Tomi Valkeinen wrote: On 11/02/14 21:07, Ryan Mallon wrote: On 12/02/14 03:06, Tomi Valkeinen wrote: On 20/09/13 10:06, Ryan Mallon wrote: Several video drivers open code the fb_write write function with code which is very similar to fb_sys_write. Replace the open code versions with calls to fb_sys_write. An fb_sync callback is added to each of the drivers. Signed-off-by: Ryan Mallon rmal...@gmail.com --- Doesn't this change the behavior so that fb_write does no longer update the display, but fb_sync does? I don't think fb_sync is even meant to update the display, it's meant to wait for an update to finish. Then again, I'm not sure about that, all I see in fb.h is wait for blit idle, optional fb_write() in fbmem.c calls -fb_sync() after -fb_write(), and I've set the fb_sync() for each of the drivers, so the behaviour should be unchanged for writes. The fb_sync() function is also called by fb_read() and fb_get_buffer_offset() (if FB_PIXMAP_SYNC flag is set). I don't know if that will adversely affect behaviour. Well, by just looking at the function names the drivers' fb_syncs call, it sounds to me that with your patch fb_sync will update the LCD, i.e. send data to it. Doing that in fb_read sounds totally wrong. Well, the alternative is to supply an fb_write() implementation for each driver that calls fb_sys_write(), and then updates the display. The fb_sync() additions can be removed. That would cut down the boiler-plate code, and should keep the behaviour the same. If you don't think it is worth the effort, then the patch can just be dropped. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] vsprintf: Remove %n handling
All in kernel users of %n in format strings have now been removed and
the %n directive is ignored. Remove the handling of %n so that it is
treated the same as any other invalid format string directive. Keep a
warning in place to deter new instances of %n in format strings.
Signed-off-by: Ryan Mallon
Acked-by: Kees Cook
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 10909c5..7c4c207 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -364,7 +364,6 @@ enum format_type {
FORMAT_TYPE_SHORT,
FORMAT_TYPE_UINT,
FORMAT_TYPE_INT,
- FORMAT_TYPE_NRCHARS,
FORMAT_TYPE_SIZE_T,
FORMAT_TYPE_PTRDIFF
};
@@ -1512,10 +1511,6 @@ qualifier:
return fmt - start;
/* skip alnum */
- case 'n':
- spec->type = FORMAT_TYPE_NRCHARS;
- return ++fmt - start;
-
case '%':
spec->type = FORMAT_TYPE_PERCENT_CHAR;
return ++fmt - start;
@@ -1538,6 +1533,15 @@ qualifier:
case 'u':
break;
+ case 'n':
+ /*
+* Since %n poses a greater security risk than utility, treat
+* it as an invalid format specifier. Warn about its use so
+* that new instances don't get added.
+*/
+ WARN_ONCE(1, "Please remove ignored %%n in '%s'\n", fmt);
+ /* Fall-through */
+
default:
spec->type = FORMAT_TYPE_INVALID;
return fmt - start;
@@ -1711,20 +1715,6 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
++str;
break;
- case FORMAT_TYPE_NRCHARS: {
- /*
-* Since %n poses a greater security risk than
-* utility, ignore %n and skip its argument.
-*/
- void *skip_arg;
-
- WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
- old_fmt);
-
- skip_arg = va_arg(args, void *);
- break;
- }
-
default:
switch (spec.type) {
case FORMAT_TYPE_LONG_LONG:
@@ -1999,19 +1989,6 @@ do {
\
fmt++;
break;
- case FORMAT_TYPE_NRCHARS: {
- /* skip %n 's argument */
- u8 qualifier = spec.qualifier;
- void *skip_arg;
- if (qualifier == 'l')
- skip_arg = va_arg(args, long *);
- else if (_tolower(qualifier) == 'z')
- skip_arg = va_arg(args, size_t *);
- else
- skip_arg = va_arg(args, int *);
- break;
- }
-
default:
switch (spec.type) {
@@ -2170,10 +2147,6 @@ int bstr_printf(char *buf, size_t size, const char *fmt,
const u32 *bin_buf)
++str;
break;
- case FORMAT_TYPE_NRCHARS:
- /* skip */
- break;
-
default: {
unsigned long long num;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] vsprintf: ignore arguments to %n
On 29/01/14 09:51, Kees Cook wrote: > On Mon, Jan 27, 2014 at 5:02 PM, Ryan Mallon wrote: >> On 28/01/14 11:39, Kees Cook wrote: >>> If arguments are consumed without output when encountering %n, it >>> could be used to benefit or improve information leak attacks that were >>> exposed via a limited size buffer. Since %n is not used by the kernel, >>> there is no reason to make an info leak attack any easier. >> >> I was thinking more like the following. Print the warning if %n is >> detected in format_decode(), but otherwise just remove the handling of >> %n outright and treat it like any other invalid format specifier. >> Something like this completely untested patch. Thoughts? > > I'd be totally fine with it. Minor typo in the comment before the > WARN_ONCE (should be "its" instead of "it"), but otherwise looks good. > Consider it: > > Acked-by: Kees Cook > > It builds and boots fine for me, FWIW. > > -Kees > It looks like your second version already got added to Andrew's mm tree. I'm happy to repost mine with a fixed typo and proper signed-off by if you'd rather use that version. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] vsprintf: ignore arguments to %n
On 29/01/14 09:51, Kees Cook wrote: On Mon, Jan 27, 2014 at 5:02 PM, Ryan Mallon rmal...@gmail.com wrote: On 28/01/14 11:39, Kees Cook wrote: If arguments are consumed without output when encountering %n, it could be used to benefit or improve information leak attacks that were exposed via a limited size buffer. Since %n is not used by the kernel, there is no reason to make an info leak attack any easier. I was thinking more like the following. Print the warning if %n is detected in format_decode(), but otherwise just remove the handling of %n outright and treat it like any other invalid format specifier. Something like this completely untested patch. Thoughts? I'd be totally fine with it. Minor typo in the comment before the WARN_ONCE (should be its instead of it), but otherwise looks good. Consider it: Acked-by: Kees Cook keesc...@chromium.org It builds and boots fine for me, FWIW. -Kees It looks like your second version already got added to Andrew's mm tree. I'm happy to repost mine with a fixed typo and proper signed-off by if you'd rather use that version. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] vsprintf: Remove %n handling
All in kernel users of %n in format strings have now been removed and
the %n directive is ignored. Remove the handling of %n so that it is
treated the same as any other invalid format string directive. Keep a
warning in place to deter new instances of %n in format strings.
Signed-off-by: Ryan Mallon rmal...@gmail.com
Acked-by: Kees Cook keesc...@chromium.org
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 10909c5..7c4c207 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -364,7 +364,6 @@ enum format_type {
FORMAT_TYPE_SHORT,
FORMAT_TYPE_UINT,
FORMAT_TYPE_INT,
- FORMAT_TYPE_NRCHARS,
FORMAT_TYPE_SIZE_T,
FORMAT_TYPE_PTRDIFF
};
@@ -1512,10 +1511,6 @@ qualifier:
return fmt - start;
/* skip alnum */
- case 'n':
- spec-type = FORMAT_TYPE_NRCHARS;
- return ++fmt - start;
-
case '%':
spec-type = FORMAT_TYPE_PERCENT_CHAR;
return ++fmt - start;
@@ -1538,6 +1533,15 @@ qualifier:
case 'u':
break;
+ case 'n':
+ /*
+* Since %n poses a greater security risk than utility, treat
+* it as an invalid format specifier. Warn about its use so
+* that new instances don't get added.
+*/
+ WARN_ONCE(1, Please remove ignored %%n in '%s'\n, fmt);
+ /* Fall-through */
+
default:
spec-type = FORMAT_TYPE_INVALID;
return fmt - start;
@@ -1711,20 +1715,6 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
++str;
break;
- case FORMAT_TYPE_NRCHARS: {
- /*
-* Since %n poses a greater security risk than
-* utility, ignore %n and skip its argument.
-*/
- void *skip_arg;
-
- WARN_ONCE(1, Please remove ignored %%n in '%s'\n,
- old_fmt);
-
- skip_arg = va_arg(args, void *);
- break;
- }
-
default:
switch (spec.type) {
case FORMAT_TYPE_LONG_LONG:
@@ -1999,19 +1989,6 @@ do {
\
fmt++;
break;
- case FORMAT_TYPE_NRCHARS: {
- /* skip %n 's argument */
- u8 qualifier = spec.qualifier;
- void *skip_arg;
- if (qualifier == 'l')
- skip_arg = va_arg(args, long *);
- else if (_tolower(qualifier) == 'z')
- skip_arg = va_arg(args, size_t *);
- else
- skip_arg = va_arg(args, int *);
- break;
- }
-
default:
switch (spec.type) {
@@ -2170,10 +2147,6 @@ int bstr_printf(char *buf, size_t size, const char *fmt,
const u32 *bin_buf)
++str;
break;
- case FORMAT_TYPE_NRCHARS:
- /* skip */
- break;
-
default: {
unsigned long long num;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] vsprintf: ignore arguments to %n
On 28/01/14 11:39, Kees Cook wrote:
> If arguments are consumed without output when encountering %n, it
> could be used to benefit or improve information leak attacks that were
> exposed via a limited size buffer. Since %n is not used by the kernel,
> there is no reason to make an info leak attack any easier.
I was thinking more like the following. Print the warning if %n is
detected in format_decode(), but otherwise just remove the handling of
%n outright and treat it like any other invalid format specifier.
Something like this completely untested patch. Thoughts?
~Ryan
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 10909c5..4e24009 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -364,7 +364,6 @@ enum format_type {
FORMAT_TYPE_SHORT,
FORMAT_TYPE_UINT,
FORMAT_TYPE_INT,
- FORMAT_TYPE_NRCHARS,
FORMAT_TYPE_SIZE_T,
FORMAT_TYPE_PTRDIFF
};
@@ -1512,10 +1511,6 @@ qualifier:
return fmt - start;
/* skip alnum */
- case 'n':
- spec->type = FORMAT_TYPE_NRCHARS;
- return ++fmt - start;
-
case '%':
spec->type = FORMAT_TYPE_PERCENT_CHAR;
return ++fmt - start;
@@ -1538,6 +1533,15 @@ qualifier:
case 'u':
break;
+ case 'n':
+ /*
+* Since %n poses a greater security risk than utility, treat
+* it as an invalid format specifier. Warn about it use, so
+* new instances don't get added.
+*/
+ WARN_ONCE(1, "Please remove ignored %%n in '%s'\n", fmt);
+ /* Fall-through */
+
default:
spec->type = FORMAT_TYPE_INVALID;
return fmt - start;
@@ -1711,20 +1715,6 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
++str;
break;
- case FORMAT_TYPE_NRCHARS: {
- /*
-* Since %n poses a greater security risk than
-* utility, ignore %n and skip its argument.
-*/
- void *skip_arg;
-
- WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
- old_fmt);
-
- skip_arg = va_arg(args, void *);
- break;
- }
-
default:
switch (spec.type) {
case FORMAT_TYPE_LONG_LONG:
@@ -1999,19 +1989,6 @@ do {
\
fmt++;
break;
- case FORMAT_TYPE_NRCHARS: {
- /* skip %n 's argument */
- u8 qualifier = spec.qualifier;
- void *skip_arg;
- if (qualifier == 'l')
- skip_arg = va_arg(args, long *);
- else if (_tolower(qualifier) == 'z')
- skip_arg = va_arg(args, size_t *);
- else
- skip_arg = va_arg(args, int *);
- break;
- }
-
default:
switch (spec.type) {
@@ -2170,10 +2147,6 @@ int bstr_printf(char *buf, size_t size, const char *fmt,
const u32 *bin_buf)
++str;
break;
- case FORMAT_TYPE_NRCHARS:
- /* skip */
- break;
-
default: {
unsigned long long num;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] vsprintf: BUG on %n
On 28/01/14 10:56, Kees Cook wrote: > On Mon, Jan 27, 2014 at 3:11 PM, Ryan Mallon wrote: >> On 28/01/14 10:03, Kees Cook wrote: >>> Now that there has been a full release of the kernel, and all users >>> of %n have been dropped, switch to %n use triggering a BUG. Ignoring >>> arguments could be used to assist in information leaks if an arbitrary >>> format string was under the control of an attacker. >> >> Not sure I follow the reasoning. %n no longer does anything in the >> kernel, so there is no risk if it does manage to find its way into a >> printed string. BUG() is for unrecoverable errors, which this clearly isn't. >> >> Information leaks via injectable strings are still possible if an >> attacker can insert %x, %d, etc. %n is more problematic since it allows >> for code injection, which is why it got removed. %n is not however, >> required to get an infoleak via a format string, so I think the summary >> is also a bit misleading. > > Yeah, I'm a bit uncomfortable with the BUG() too. The issue with %n is > that it would produce no output at all to skip arguments. With other > things, you have to take up output space, which may be limited. How > about just not skipping the argument? Leave the warn_on, etc? If you are trying to catch in kernel users of %n, then the warning is probably fine. I don't think the presense of a %n in a format string, without any injection vulnerability is going to cause a problem. If you are trying to catch %n being injected by a malicious user into a vulnerable string then a warning is fine as long as the string doesn't allow code injection through some other means. I don't think you can easily prevent infoleaks at runtime, since any vulnerable can have %x, %s, or whatever injected to leak information on the stack. There was some work on detecting potentially vulnerable strings at compile time I think? The reason to get rid of %n is to remove the ability to escalate an infoleak on a vulnerable format string into code execution. Vulnerable strings and infoleaks via them are really a separate issue, and detecting %n does nothing to solve them. %n should probably just be treated the same as any other %FOO which is not a valid format string directive. Keeping the warning might be useful for kernel developers who don't know that they shouldn't be using it. Then again, sparse, checkpatch or code review might be the better place to do that. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] vsprintf: BUG on %n
On 28/01/14 10:03, Kees Cook wrote:
> Now that there has been a full release of the kernel, and all users
> of %n have been dropped, switch to %n use triggering a BUG. Ignoring
> arguments could be used to assist in information leaks if an arbitrary
> format string was under the control of an attacker.
Not sure I follow the reasoning. %n no longer does anything in the
kernel, so there is no risk if it does manage to find its way into a
printed string. BUG() is for unrecoverable errors, which this clearly isn't.
Information leaks via injectable strings are still possible if an
attacker can insert %x, %d, etc. %n is more problematic since it allows
for code injection, which is why it got removed. %n is not however,
required to get an infoleak via a format string, so I think the summary
is also a bit misleading.
~Ryan
>
> Signed-off-by: Kees Cook
> ---
> lib/vsprintf.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 185b6d300ebc..a27fd7f61325 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1735,15 +1735,12 @@ int vsnprintf(char *buf, size_t size, const char
> *fmt, va_list args)
> case FORMAT_TYPE_NRCHARS: {
> /*
>* Since %n poses a greater security risk than
> - * utility, ignore %n and skip its argument.
> + * utility, it should not be implemented. Instead,
> + * BUG when encountering %n, since there are no
> + * legitimate users and skipping arguments could
> + * assist information leak attacks.
>*/
> - void *skip_arg;
> -
> - WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
> - old_fmt);
> -
> - skip_arg = va_arg(args, void *);
> - break;
> + BUG();
> }
>
> default:
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] vsprintf: BUG on %n
On 28/01/14 10:03, Kees Cook wrote:
Now that there has been a full release of the kernel, and all users
of %n have been dropped, switch to %n use triggering a BUG. Ignoring
arguments could be used to assist in information leaks if an arbitrary
format string was under the control of an attacker.
Not sure I follow the reasoning. %n no longer does anything in the
kernel, so there is no risk if it does manage to find its way into a
printed string. BUG() is for unrecoverable errors, which this clearly isn't.
Information leaks via injectable strings are still possible if an
attacker can insert %x, %d, etc. %n is more problematic since it allows
for code injection, which is why it got removed. %n is not however,
required to get an infoleak via a format string, so I think the summary
is also a bit misleading.
~Ryan
Signed-off-by: Kees Cook keesc...@chromium.org
---
lib/vsprintf.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 185b6d300ebc..a27fd7f61325 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1735,15 +1735,12 @@ int vsnprintf(char *buf, size_t size, const char
*fmt, va_list args)
case FORMAT_TYPE_NRCHARS: {
/*
* Since %n poses a greater security risk than
- * utility, ignore %n and skip its argument.
+ * utility, it should not be implemented. Instead,
+ * BUG when encountering %n, since there are no
+ * legitimate users and skipping arguments could
+ * assist information leak attacks.
*/
- void *skip_arg;
-
- WARN_ONCE(1, Please remove ignored %%n in '%s'\n,
- old_fmt);
-
- skip_arg = va_arg(args, void *);
- break;
+ BUG();
}
default:
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] vsprintf: BUG on %n
On 28/01/14 10:56, Kees Cook wrote: On Mon, Jan 27, 2014 at 3:11 PM, Ryan Mallon rmal...@gmail.com wrote: On 28/01/14 10:03, Kees Cook wrote: Now that there has been a full release of the kernel, and all users of %n have been dropped, switch to %n use triggering a BUG. Ignoring arguments could be used to assist in information leaks if an arbitrary format string was under the control of an attacker. Not sure I follow the reasoning. %n no longer does anything in the kernel, so there is no risk if it does manage to find its way into a printed string. BUG() is for unrecoverable errors, which this clearly isn't. Information leaks via injectable strings are still possible if an attacker can insert %x, %d, etc. %n is more problematic since it allows for code injection, which is why it got removed. %n is not however, required to get an infoleak via a format string, so I think the summary is also a bit misleading. Yeah, I'm a bit uncomfortable with the BUG() too. The issue with %n is that it would produce no output at all to skip arguments. With other things, you have to take up output space, which may be limited. How about just not skipping the argument? Leave the warn_on, etc? If you are trying to catch in kernel users of %n, then the warning is probably fine. I don't think the presense of a %n in a format string, without any injection vulnerability is going to cause a problem. If you are trying to catch %n being injected by a malicious user into a vulnerable string then a warning is fine as long as the string doesn't allow code injection through some other means. I don't think you can easily prevent infoleaks at runtime, since any vulnerable can have %x, %s, or whatever injected to leak information on the stack. There was some work on detecting potentially vulnerable strings at compile time I think? The reason to get rid of %n is to remove the ability to escalate an infoleak on a vulnerable format string into code execution. Vulnerable strings and infoleaks via them are really a separate issue, and detecting %n does nothing to solve them. %n should probably just be treated the same as any other %FOO which is not a valid format string directive. Keeping the warning might be useful for kernel developers who don't know that they shouldn't be using it. Then again, sparse, checkpatch or code review might be the better place to do that. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] vsprintf: ignore arguments to %n
On 28/01/14 11:39, Kees Cook wrote:
If arguments are consumed without output when encountering %n, it
could be used to benefit or improve information leak attacks that were
exposed via a limited size buffer. Since %n is not used by the kernel,
there is no reason to make an info leak attack any easier.
I was thinking more like the following. Print the warning if %n is
detected in format_decode(), but otherwise just remove the handling of
%n outright and treat it like any other invalid format specifier.
Something like this completely untested patch. Thoughts?
~Ryan
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 10909c5..4e24009 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -364,7 +364,6 @@ enum format_type {
FORMAT_TYPE_SHORT,
FORMAT_TYPE_UINT,
FORMAT_TYPE_INT,
- FORMAT_TYPE_NRCHARS,
FORMAT_TYPE_SIZE_T,
FORMAT_TYPE_PTRDIFF
};
@@ -1512,10 +1511,6 @@ qualifier:
return fmt - start;
/* skip alnum */
- case 'n':
- spec-type = FORMAT_TYPE_NRCHARS;
- return ++fmt - start;
-
case '%':
spec-type = FORMAT_TYPE_PERCENT_CHAR;
return ++fmt - start;
@@ -1538,6 +1533,15 @@ qualifier:
case 'u':
break;
+ case 'n':
+ /*
+* Since %n poses a greater security risk than utility, treat
+* it as an invalid format specifier. Warn about it use, so
+* new instances don't get added.
+*/
+ WARN_ONCE(1, Please remove ignored %%n in '%s'\n, fmt);
+ /* Fall-through */
+
default:
spec-type = FORMAT_TYPE_INVALID;
return fmt - start;
@@ -1711,20 +1715,6 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
++str;
break;
- case FORMAT_TYPE_NRCHARS: {
- /*
-* Since %n poses a greater security risk than
-* utility, ignore %n and skip its argument.
-*/
- void *skip_arg;
-
- WARN_ONCE(1, Please remove ignored %%n in '%s'\n,
- old_fmt);
-
- skip_arg = va_arg(args, void *);
- break;
- }
-
default:
switch (spec.type) {
case FORMAT_TYPE_LONG_LONG:
@@ -1999,19 +1989,6 @@ do {
\
fmt++;
break;
- case FORMAT_TYPE_NRCHARS: {
- /* skip %n 's argument */
- u8 qualifier = spec.qualifier;
- void *skip_arg;
- if (qualifier == 'l')
- skip_arg = va_arg(args, long *);
- else if (_tolower(qualifier) == 'z')
- skip_arg = va_arg(args, size_t *);
- else
- skip_arg = va_arg(args, int *);
- break;
- }
-
default:
switch (spec.type) {
@@ -2170,10 +2147,6 @@ int bstr_printf(char *buf, size_t size, const char *fmt,
const u32 *bin_buf)
++str;
break;
- case FORMAT_TYPE_NRCHARS:
- /* skip */
- break;
-
default: {
unsigned long long num;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] Known exploit detection
On 13/12/13 06:06, Theodore Ts'o wrote: > On Thu, Dec 12, 2013 at 05:52:24PM +0100, vegard.nos...@oracle.com wrote: >> From: Vegard Nossum >> >> The idea is simple -- since different kernel versions are vulnerable to >> different root exploits, hackers most likely try multiple exploits before >> they actually succeed. > > Suppose we put put this into the mainstream kernel. Wouldn't writers > of root kit adapt by checking for the kernel version to avoid checking > for exploits that are known not work? So the question is whether the > additional complexity in the kernel is going to be worth it, since > once the attackers adapt, the benefits of trying to detect attacks for > mitigated exploits will be minimal. Doesn't the fact that the exploits are already mitigated make it of limited value anyway? In order for this detection to be effective, a system must be fully patched with all the latest CVE tags (and also, obviously all the associated security patches), otherwise the system will be vulnerable to the most recent security bugs, and will be unable to warn about them. If the system is fully patched, and an attacker is only using known attacks, then they aren't getting in anyway. The logging might be of some use in identifying users who are potentially malicious, but then those users are low threat anyway since all the attacks they are trying are fixed. Is it worth all the instrumentation of the kernel for this? So I think for the most serious cases of attack, where the attacker has some knowledge of the system version/patch level (for a system which is not fully patched), or has zero-day vulnerabilities, this protection will do nothing. This doesn't really work as a protection mechanism, the idea that "hackers most likely try multiple exploits before they actually succeed." seems a bit flawed. If they are eventually succeeding using a known vulnerability against an unpatched system, this this patchset is of limited protection. If the attacker is smart about the order of the attacks (e.g. try the new ones first) then they can probably still get in without triggering warnings. Sophisticated attackers who are have unpatched, unknown vulnerabilities, but still want to use known ones where possible are probably smart enough to evade any sort of protection mechanism like this. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] Known exploit detection
On 13/12/13 06:06, Theodore Ts'o wrote: On Thu, Dec 12, 2013 at 05:52:24PM +0100, vegard.nos...@oracle.com wrote: From: Vegard Nossum vegard.nos...@oracle.com The idea is simple -- since different kernel versions are vulnerable to different root exploits, hackers most likely try multiple exploits before they actually succeed. Suppose we put put this into the mainstream kernel. Wouldn't writers of root kit adapt by checking for the kernel version to avoid checking for exploits that are known not work? So the question is whether the additional complexity in the kernel is going to be worth it, since once the attackers adapt, the benefits of trying to detect attacks for mitigated exploits will be minimal. Doesn't the fact that the exploits are already mitigated make it of limited value anyway? In order for this detection to be effective, a system must be fully patched with all the latest CVE tags (and also, obviously all the associated security patches), otherwise the system will be vulnerable to the most recent security bugs, and will be unable to warn about them. If the system is fully patched, and an attacker is only using known attacks, then they aren't getting in anyway. The logging might be of some use in identifying users who are potentially malicious, but then those users are low threat anyway since all the attacks they are trying are fixed. Is it worth all the instrumentation of the kernel for this? So I think for the most serious cases of attack, where the attacker has some knowledge of the system version/patch level (for a system which is not fully patched), or has zero-day vulnerabilities, this protection will do nothing. This doesn't really work as a protection mechanism, the idea that hackers most likely try multiple exploits before they actually succeed. seems a bit flawed. If they are eventually succeeding using a known vulnerability against an unpatched system, this this patchset is of limited protection. If the attacker is smart about the order of the attacks (e.g. try the new ones first) then they can probably still get in without triggering warnings. Sophisticated attackers who are have unpatched, unknown vulnerabilities, but still want to use known ones where possible are probably smart enough to evade any sort of protection mechanism like this. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] Known exploit detection
On 14/12/13 00:06, Ingo Molnar wrote: > > * Ryan Mallon wrote: > >> On 13/12/13 08:13, Kees Cook wrote: >>> On Thu, Dec 12, 2013 at 11:06 AM, Theodore Ts'o wrote: >>>> On Thu, Dec 12, 2013 at 05:52:24PM +0100, vegard.nos...@oracle.com wrote: >>>>> From: Vegard Nossum >>>>> >>>>> The idea is simple -- since different kernel versions are vulnerable to >>>>> different root exploits, hackers most likely try multiple exploits before >>>>> they actually succeed. >>> >>> I like this idea. It serves a few purposes, not the least of which is >>> very clearly marking in code where we've had problems, regardless of >>> the fact that it reports badness to the system owner. And I think >>> getting any additional notifications about bad behavior is a nice idea >>> too. >> >> Though, if an attacker is running through a series of exploits, and >> one eventually succeeds then the first thing to do would be to clean >> traces of the _exploit() notifications from the syslog. [...] > > There are several solutions to that: > > 1) > > Critical sites use remote logging over a fast LAN, so a successful > exploit would have to zap the remote logging daemon pretty quickly > before the log message goes out over the network. > > 2) > > Some sites also log to append-only media [such as a printer] or other > append-only storage interfaces - which cannot be manipulated from the > attacked system alone after a successful break-in. > > 3) > > In future the exploit() code could trigger actual active defensive > measures, such as immediately freezing all tasks of that UID and > blocking further fork()s/exec()s of that UID. > > Depending on how critical the security of the system is, such active > measures might still be a preferable outcome even if there's a chance > of false positives. (Such active measures that freeze the UID will > also help with forensics, if the attack is indeed real.) > >> [...] Since running through a series of exploits is pretty quick, >> this can probably all be done before the sysadmin ever notices. > > It's not necessarily the sysadmin the attacker is racing against, but > against append-only logging and other defensive measures - which too > are programs. > >> The _exploit() notifications could also be used to spam the syslogs. >> Although they are individually ratelimited, if there are enough >> _exploit() markers in the kernel then an annoying person can cycle >> through them all to generate large amounts of useless syslog. > > AFAICS they are globally rate-limited, just like many other > attacker-triggerable printk()s the kernel may generate. Actually, that opens another possibility for an attacker. Since they know the logging is rate-limited, they can first attempt a low risk CVE (or one that is known to have false positives, see comments from others about dumb user-space/corrupted file-systems, etc). So that attempt gets logged, and possibly ignored, but then more attempts can be quickly made that will not be logged. Of course, if you have some policy that kicks the user or some such then that would limit this approach, but just rate-limited logging by itself might not be enough. The problem with policy based solutions, though, is that if the attacker knows what the policy is then they can game it. E.g. don't bother trying the known exploits which will immediately get you kicked; add sufficient delay between attempts, etc. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] Known exploit detection
On 13/12/13 20:20, Vegard Nossum wrote:
> On 12/13/2013 12:50 AM, Ryan Mallon wrote:
>> On 13/12/13 08:13, Kees Cook wrote:
>>> On Thu, Dec 12, 2013 at 11:06 AM, Theodore Ts'o wrote:
>>>> On Thu, Dec 12, 2013 at 05:52:24PM +0100, vegard.nos...@oracle.com wrote:
>>>>> The idea is simple -- since different kernel versions are vulnerable to
>>>>> different root exploits, hackers most likely try multiple exploits before
>>>>> they actually succeed.
>>
>> The _exploit() notifications could also be used to spam the syslogs.
>> Although they are individually ratelimited, if there are enough
>> _exploit() markers in the kernel then an annoying person can cycle
>> through them all to generate large amounts of useless syslog.
>
> They are rate limited collectively, not individually, so this should not be
> an issue.
Yes, sorry, I misread the code.
I wonder if the exploit() function name should be changed though. Having:
exploit("CVE-");
In the code looks like some sort of injection/testing framework. Maybe:
warn_known_exploit("CVE-");
would be clearer?
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] Known exploit detection
On 13/12/13 20:20, Vegard Nossum wrote: On 12/13/2013 12:50 AM, Ryan Mallon wrote: On 13/12/13 08:13, Kees Cook wrote: On Thu, Dec 12, 2013 at 11:06 AM, Theodore Ts'o ty...@mit.edu wrote: On Thu, Dec 12, 2013 at 05:52:24PM +0100, vegard.nos...@oracle.com wrote: The idea is simple -- since different kernel versions are vulnerable to different root exploits, hackers most likely try multiple exploits before they actually succeed. The _exploit() notifications could also be used to spam the syslogs. Although they are individually ratelimited, if there are enough _exploit() markers in the kernel then an annoying person can cycle through them all to generate large amounts of useless syslog. They are rate limited collectively, not individually, so this should not be an issue. Yes, sorry, I misread the code. I wonder if the exploit() function name should be changed though. Having: exploit(CVE-); In the code looks like some sort of injection/testing framework. Maybe: warn_known_exploit(CVE-); would be clearer? ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] Known exploit detection
On 14/12/13 00:06, Ingo Molnar wrote: * Ryan Mallon rmal...@gmail.com wrote: On 13/12/13 08:13, Kees Cook wrote: On Thu, Dec 12, 2013 at 11:06 AM, Theodore Ts'o ty...@mit.edu wrote: On Thu, Dec 12, 2013 at 05:52:24PM +0100, vegard.nos...@oracle.com wrote: From: Vegard Nossum vegard.nos...@oracle.com The idea is simple -- since different kernel versions are vulnerable to different root exploits, hackers most likely try multiple exploits before they actually succeed. I like this idea. It serves a few purposes, not the least of which is very clearly marking in code where we've had problems, regardless of the fact that it reports badness to the system owner. And I think getting any additional notifications about bad behavior is a nice idea too. Though, if an attacker is running through a series of exploits, and one eventually succeeds then the first thing to do would be to clean traces of the _exploit() notifications from the syslog. [...] There are several solutions to that: 1) Critical sites use remote logging over a fast LAN, so a successful exploit would have to zap the remote logging daemon pretty quickly before the log message goes out over the network. 2) Some sites also log to append-only media [such as a printer] or other append-only storage interfaces - which cannot be manipulated from the attacked system alone after a successful break-in. 3) In future the exploit() code could trigger actual active defensive measures, such as immediately freezing all tasks of that UID and blocking further fork()s/exec()s of that UID. Depending on how critical the security of the system is, such active measures might still be a preferable outcome even if there's a chance of false positives. (Such active measures that freeze the UID will also help with forensics, if the attack is indeed real.) [...] Since running through a series of exploits is pretty quick, this can probably all be done before the sysadmin ever notices. It's not necessarily the sysadmin the attacker is racing against, but against append-only logging and other defensive measures - which too are programs. The _exploit() notifications could also be used to spam the syslogs. Although they are individually ratelimited, if there are enough _exploit() markers in the kernel then an annoying person can cycle through them all to generate large amounts of useless syslog. AFAICS they are globally rate-limited, just like many other attacker-triggerable printk()s the kernel may generate. Actually, that opens another possibility for an attacker. Since they know the logging is rate-limited, they can first attempt a low risk CVE (or one that is known to have false positives, see comments from others about dumb user-space/corrupted file-systems, etc). So that attempt gets logged, and possibly ignored, but then more attempts can be quickly made that will not be logged. Of course, if you have some policy that kicks the user or some such then that would limit this approach, but just rate-limited logging by itself might not be enough. The problem with policy based solutions, though, is that if the attacker knows what the policy is then they can game it. E.g. don't bother trying the known exploits which will immediately get you kicked; add sufficient delay between attempts, etc. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] Known exploit detection
On 13/12/13 08:13, Kees Cook wrote: > On Thu, Dec 12, 2013 at 11:06 AM, Theodore Ts'o wrote: >> On Thu, Dec 12, 2013 at 05:52:24PM +0100, vegard.nos...@oracle.com wrote: >>> From: Vegard Nossum >>> >>> The idea is simple -- since different kernel versions are vulnerable to >>> different root exploits, hackers most likely try multiple exploits before >>> they actually succeed. > > I like this idea. It serves a few purposes, not the least of which is > very clearly marking in code where we've had problems, regardless of > the fact that it reports badness to the system owner. And I think > getting any additional notifications about bad behavior is a nice idea > too. Though, if an attacker is running through a series of exploits, and one eventually succeeds then the first thing to do would be to clean traces of the _exploit() notifications from the syslog. Since running through a series of exploits is pretty quick, this can probably all be done before the sysadmin ever notices. The _exploit() notifications could also be used to spam the syslogs. Although they are individually ratelimited, if there are enough _exploit() markers in the kernel then an annoying person can cycle through them all to generate large amounts of useless syslog. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] Known exploit detection
On 13/12/13 08:13, Kees Cook wrote: On Thu, Dec 12, 2013 at 11:06 AM, Theodore Ts'o ty...@mit.edu wrote: On Thu, Dec 12, 2013 at 05:52:24PM +0100, vegard.nos...@oracle.com wrote: From: Vegard Nossum vegard.nos...@oracle.com The idea is simple -- since different kernel versions are vulnerable to different root exploits, hackers most likely try multiple exploits before they actually succeed. I like this idea. It serves a few purposes, not the least of which is very clearly marking in code where we've had problems, regardless of the fact that it reports badness to the system owner. And I think getting any additional notifications about bad behavior is a nice idea too. Though, if an attacker is running through a series of exploits, and one eventually succeeds then the first thing to do would be to clean traces of the _exploit() notifications from the syslog. Since running through a series of exploits is pretty quick, this can probably all be done before the sysadmin ever notices. The _exploit() notifications could also be used to spam the syslogs. Although they are individually ratelimited, if there are enough _exploit() markers in the kernel then an annoying person can cycle through them all to generate large amounts of useless syslog. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -next 1/3] seq: Add a seq_overflow test.
On 11/12/13 16:12, Joe Perches wrote:
> seq_printf and seq_puts returns are often misused.
>
> Instead of checking the seq_printf or seq_puts return,
> add a new seq_overflow function to test if a seq_file has
> overflowed the available buffer space.
>
> This will eventually allow seq_printf and seq_puts to be
> converted to have a void return instead of the int return
> that is often assumed to have a size_t value instead of an
> error/no-error value.
>
> Signed-off-by: Joe Perches
> ---
> fs/seq_file.c| 15 ---
> include/linux/seq_file.h | 2 ++
> 2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 1d641bb..aab0736 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -14,16 +14,17 @@
> #include
> #include
>
> -
> -/*
> - * seq_files have a buffer which can may overflow. When this happens a larger
> - * buffer is reallocated and all the data will be printed again.
> - * The overflow state is true when m->count == m->size.
> +/**
> + * seq_overflow - test if a seq_file has overflowed the space available
> + * @m: the seq_file handle
> + *
> + * Returns -1 when an overflow has occurred, 0 otherwise.
> */
> -static bool seq_overflow(struct seq_file *m)
> +int seq_overflow(struct seq_file *m)
> {
> - return m->count == m->size;
> + return m->count == m->size ? -1 : 0;
> }
> +EXPORT_SYMBOL(seq_overflow);
What is the reasoning in making this return int instead of bool? Having
it return int encourages people to do:
return seq_overflow(s);
When used in seq_file functions will return -EPERM (-1) to user-space,
which is confusing. It should probably return bool and let the caller
sort out the correct error to return.
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -next 1/3] seq: Add a seq_overflow test.
On 11/12/13 16:12, Joe Perches wrote:
seq_printf and seq_puts returns are often misused.
Instead of checking the seq_printf or seq_puts return,
add a new seq_overflow function to test if a seq_file has
overflowed the available buffer space.
This will eventually allow seq_printf and seq_puts to be
converted to have a void return instead of the int return
that is often assumed to have a size_t value instead of an
error/no-error value.
Signed-off-by: Joe Perches j...@perches.com
---
fs/seq_file.c| 15 ---
include/linux/seq_file.h | 2 ++
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1d641bb..aab0736 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -14,16 +14,17 @@
#include asm/uaccess.h
#include asm/page.h
-
-/*
- * seq_files have a buffer which can may overflow. When this happens a larger
- * buffer is reallocated and all the data will be printed again.
- * The overflow state is true when m-count == m-size.
+/**
+ * seq_overflow - test if a seq_file has overflowed the space available
+ * @m: the seq_file handle
+ *
+ * Returns -1 when an overflow has occurred, 0 otherwise.
*/
-static bool seq_overflow(struct seq_file *m)
+int seq_overflow(struct seq_file *m)
{
- return m-count == m-size;
+ return m-count == m-size ? -1 : 0;
}
+EXPORT_SYMBOL(seq_overflow);
What is the reasoning in making this return int instead of bool? Having
it return int encourages people to do:
return seq_overflow(s);
When used in seq_file functions will return -EPERM (-1) to user-space,
which is confusing. It should probably return bool and let the caller
sort out the correct error to return.
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 7/9] mm: thrash detection-based file cache sizing
On 25/11/13 10:38, Johannes Weiner wrote: > The VM maintains cached filesystem pages on two types of lists. One > list holds the pages recently faulted into the cache, the other list > holds pages that have been referenced repeatedly on that first list. > The idea is to prefer reclaiming young pages over those that have > shown to benefit from caching in the past. We call the recently used > list "inactive list" and the frequently used list "active list". > > Currently, the VM aims for a 1:1 ratio between the lists, which is the > "perfect" trade-off between the ability to *protect* frequently used > pages and the ability to *detect* frequently used pages. This means > that working set changes bigger than half of cache memory go > undetected and thrash indefinitely, whereas working sets bigger than > half of cache memory are unprotected against used-once streams that > don't even need caching. > > Historically, every reclaim scan of the inactive list also took a > smaller number of pages from the tail of the active list and moved > them to the head of the inactive list. This model gave established > working sets more gracetime in the face of temporary use-once streams, > but ultimately was not significantly better than a FIFO policy and > still thrashed cache based on eviction speed, rather than actual > demand for cache. > > This patch solves one half of the problem by decoupling the ability to > detect working set changes from the inactive list size. By > maintaining a history of recently evicted file pages it can detect > frequently used pages with an arbitrarily small inactive list size, > and subsequently apply pressure on the active list based on actual > demand for cache, not just overall eviction speed. > > Every zone maintains a counter that tracks inactive list aging speed. > When a page is evicted, a snapshot of this counter is stored in the > now-empty page cache radix tree slot. On refault, the minimum access > distance of the page can be assesed, to evaluate whether the page > should be part of the active list or not. > > This fixes the VM's blindness towards working set changes in excess of > the inactive list. And it's the foundation to further improve the > protection ability and reduce the minimum inactive list size of 50%. > > Signed-off-by: Johannes Weiner > --- > + * fault + > + * | > + * +--+ |+-+ > + * reclaim <- | inactive | <-+-- demotion |active | <--+ > + * +--++-+| > + * | | > + * +-- promotion --+ > + * > + * > + * Access frequency and refault distance > + * > + * A workload is trashing when its pages are frequently used but they "thrashing". ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 7/9] mm: thrash detection-based file cache sizing
On 25/11/13 10:38, Johannes Weiner wrote: The VM maintains cached filesystem pages on two types of lists. One list holds the pages recently faulted into the cache, the other list holds pages that have been referenced repeatedly on that first list. The idea is to prefer reclaiming young pages over those that have shown to benefit from caching in the past. We call the recently used list inactive list and the frequently used list active list. Currently, the VM aims for a 1:1 ratio between the lists, which is the perfect trade-off between the ability to *protect* frequently used pages and the ability to *detect* frequently used pages. This means that working set changes bigger than half of cache memory go undetected and thrash indefinitely, whereas working sets bigger than half of cache memory are unprotected against used-once streams that don't even need caching. Historically, every reclaim scan of the inactive list also took a smaller number of pages from the tail of the active list and moved them to the head of the inactive list. This model gave established working sets more gracetime in the face of temporary use-once streams, but ultimately was not significantly better than a FIFO policy and still thrashed cache based on eviction speed, rather than actual demand for cache. This patch solves one half of the problem by decoupling the ability to detect working set changes from the inactive list size. By maintaining a history of recently evicted file pages it can detect frequently used pages with an arbitrarily small inactive list size, and subsequently apply pressure on the active list based on actual demand for cache, not just overall eviction speed. Every zone maintains a counter that tracks inactive list aging speed. When a page is evicted, a snapshot of this counter is stored in the now-empty page cache radix tree slot. On refault, the minimum access distance of the page can be assesed, to evaluate whether the page should be part of the active list or not. This fixes the VM's blindness towards working set changes in excess of the inactive list. And it's the foundation to further improve the protection ability and reduce the minimum inactive list size of 50%. Signed-off-by: Johannes Weiner han...@cmpxchg.org --- snip + * fault + + * | + * +--+ |+-+ + * reclaim - | inactive | -+-- demotion |active | --+ + * +--++-+| + * | | + * +-- promotion --+ + * + * + * Access frequency and refault distance + * + * A workload is trashing when its pages are frequently used but they thrashing. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/6] GenWQE Debugfs interfaces
On 05/11/13 19:44, Frank Haverkamp wrote: > Debugfs interfaces for the GenWQE card. Help to debug potential > problems. Dump internal chip state for debugging and failure > determination. > > Signed-off-by: Frank Haverkamp > Co-authors: Joerg-Stephan Vogt , > Michael Jung , > Michael Ruettger Couple of comments below. ~Ryan > --- > Documentation/ABI/testing/debugfs-driver-genwqe | 70 +++ > drivers/misc/genwqe/card_debugfs.c | 579 > +++ > 2 files changed, 649 insertions(+), 0 deletions(-) > create mode 100644 Documentation/ABI/testing/debugfs-driver-genwqe > create mode 100644 drivers/misc/genwqe/card_debugfs.c > > diff --git a/Documentation/ABI/testing/debugfs-driver-genwqe > b/Documentation/ABI/testing/debugfs-driver-genwqe > new file mode 100644 > index 000..548883a > --- /dev/null > +++ b/Documentation/ABI/testing/debugfs-driver-genwqe > @@ -0,0 +1,70 @@ > +What: /sys/kernel/debug/genwqe/genwqe_card/ddcb_info > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:DDCB queue dump used for debugging queueing problems. > + > +What: /sys/kernel/debug/genwqe/genwqe_card/curr_regs > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Dump of the current error registers. > + > +What: /sys/kernel/debug/genwqe/genwqe_card/curr_uid0 > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Internal chip state of UID0 (unit id 0). > + > +What: /sys/kernel/debug/genwqe/genwqe_card/curr_uid1 > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Internal chip state of UID1. > + > +What: /sys/kernel/debug/genwqe/genwqe_card/curr_uid2 > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Internal chip state of UID2. > + > +What: /sys/kernel/debug/genwqe/genwqe_card/prev_regs > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Dump of the error registers before the last reset of > +the card occured. > + > +What: /sys/kernel/debug/genwqe/genwqe_card/prev_uid0 > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Internal chip state of UID0 before card was reset. > + > +What: /sys/kernel/debug/genwqe/genwqe_card/prev_uid1 > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Internal chip state of UID1 before card was reset. > + > +What: /sys/kernel/debug/genwqe/genwqe_card/prev_uid2 > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Internal chip state of UID2 before card was reset. > + > +What: /sys/kernel/debug/genwqe/genwqe_card/info > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Comprehensive summary of bitstream version and software > +version. Used bitstream and bitstream clocking information. > + > +What: /sys/kernel/debug/genwqe/genwqe_card/err_inject > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Possibility to inject error cases to ensure that the drivers > +error handling code works well. > + > +What: > /sys/kernel/debug/genwqe/genwqe_card/vf<0..14>_jobtimeout_msec > +Date: Oct 2013 > +Contact:ha...@linux.vnet.ibm.com > +Description:Default VF timeout 250ms. Testing might require 1000ms. > +Using 0 will use the cards default value (whatever that is). > + > +The timeout depends on the max number of available cards > +in the system and the maximum allowed queue size. > + > +The driver ensures that the settings are done just before > +the VFs get enabled. Changing the timeouts in flight is not > +possible. > diff --git a/drivers/misc/genwqe/card_debugfs.c > b/drivers/misc/genwqe/card_debugfs.c > new file mode 100644 > index 000..ebf2f93 > --- /dev/null > +++ b/drivers/misc/genwqe/card_debugfs.c > @@ -0,0 +1,579 @@ > +/** > + * IBM Accelerator Family 'GenWQE' > + * > + * (C) Copyright IBM Corp. 2013 > + * > + * Author: Frank Haverkamp > + * Author: Joerg-Stephan Vogt > + * Author: Michael Jung > + * Author: Michael Ruettger > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License (version 2 only) > + * as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +/* > + * Debugfs interfaces for the
Re: [PATCH 5/6] GenWQE Sysfs interfaces
On 05/11/13 19:44, Frank Haverkamp wrote:
> Sysfs interfaces for the GenWQE card. There are attributes to query
> the version of the bitstream as well as some for the driver. For
> debugging, please also see the debugfs interfaces of this driver.
>
> Signed-off-by: Frank Haverkamp
> Co-authors: Joerg-Stephan Vogt ,
> Michael Jung ,
> Michael Ruettger
Handful of small comments below,
~Ryan
> ---
> Documentation/ABI/testing/sysfs-driver-genwqe | 55 +
> drivers/misc/genwqe/card_sysfs.c | 290
> +
> 2 files changed, 345 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-genwqe
> create mode 100644 drivers/misc/genwqe/card_sysfs.c
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-genwqe
> b/Documentation/ABI/testing/sysfs-driver-genwqe
> new file mode 100644
> index 000..0976901
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-genwqe
> @@ -0,0 +1,55 @@
> +What: /sys/class/genwqe/genwqe_card/version
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:Unique bitstream identification e.g.
> +'000330336283.475a4950'.
> +
> +What: /sys/class/genwqe/genwqe_card/appid
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:Identifies the currently active card application e.g. 'GZIP'
> +for compression/decompression.
> +
> +What: /sys/class/genwqe/genwqe_card/type
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:Type of the card e.g. 'GenWQE5-A7'.
> +
> +What: /sys/class/genwqe/genwqe_card/cpld_version
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:Version id of the cards CPLD.
> +
> +What: /sys/class/genwqe/genwqe_card/curr_bitstream
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:Currently active bitstream. 1 is default, 0 is backup.
> +
> +What: /sys/class/genwqe/genwqe_card/next_bitstream
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:Interface to set the next bitstream to be used.
> +
> +What: /sys/class/genwqe/genwqe_card/tempsens
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:Interface to read the cards temperature sense register.
> +
> +What: /sys/class/genwqe/genwqe_card/state
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:State of the card: "unused", "used", "error".
> +
> +What: /sys/class/genwqe/genwqe_card/base_clock
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:Base clock frequency of the card.
> +
> +What: /sys/class/genwqe/genwqe_card/device/sriov_numvfs
> +Date: Oct 2013
> +Contact:ha...@linux.vnet.ibm.com
> +Description:Enable VFs (1..15):
> + sudo sh -c 'echo 15 > \
> +/sys/bus/pci/devices/\:1b\:00.0/sriov_numvfs'
> +Disable VFs:
> + Write a 0 into the same sysfs entry.
> diff --git a/drivers/misc/genwqe/card_sysfs.c
> b/drivers/misc/genwqe/card_sysfs.c
> new file mode 100644
> index 000..f29ce55
> --- /dev/null
> +++ b/drivers/misc/genwqe/card_sysfs.c
> @@ -0,0 +1,290 @@
> +/**
> + * IBM Accelerator Family 'GenWQE'
> + *
> + * (C) Copyright IBM Corp. 2013
> + *
> + * Author: Frank Haverkamp
> + * Author: Joerg-Stephan Vogt
> + * Author: Michael Jung
> + * Author: Michael Ruettger
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License (version 2 only)
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +/*
> + * Sysfs interfaces for the GenWQE card. There are attributes to query
> + * the version of the bitstream as well as some for the driver. For
> + * debugging, please also see the debugfs interfaces of this driver.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "card_base.h"
> +#include "card_ddcb.h"
> +
> +static const char * const genwqe_types[] = {
> + [GENWQE_TYPE_ALTERA_230] = "GenWQE4-230",
> + [GENWQE_TYPE_ALTERA_530] = "GenWQE4-530",
> + [GENWQE_TYPE_ALTERA_A4] = "GenWQE5-A4",
> + [GENWQE_TYPE_ALTERA_A7] = "GenWQE5-A7",
> +};
> +
> +static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> +char *buf)
>
Re: [PATCH 5/6] GenWQE Sysfs interfaces
On 05/11/13 19:44, Frank Haverkamp wrote:
Sysfs interfaces for the GenWQE card. There are attributes to query
the version of the bitstream as well as some for the driver. For
debugging, please also see the debugfs interfaces of this driver.
Signed-off-by: Frank Haverkamp ha...@linux.vnet.ibm.com
Co-authors: Joerg-Stephan Vogt jsv...@de.ibm.com,
Michael Jung mij...@de.ibm.com,
Michael Ruettger mich...@ibmra.de
Handful of small comments below,
~Ryan
---
Documentation/ABI/testing/sysfs-driver-genwqe | 55 +
drivers/misc/genwqe/card_sysfs.c | 290
+
2 files changed, 345 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-driver-genwqe
create mode 100644 drivers/misc/genwqe/card_sysfs.c
diff --git a/Documentation/ABI/testing/sysfs-driver-genwqe
b/Documentation/ABI/testing/sysfs-driver-genwqe
new file mode 100644
index 000..0976901
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-genwqe
@@ -0,0 +1,55 @@
+What: /sys/class/genwqe/genwqen_card/version
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:Unique bitstream identification e.g.
+'000330336283.475a4950'.
+
+What: /sys/class/genwqe/genwqen_card/appid
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:Identifies the currently active card application e.g. 'GZIP'
+for compression/decompression.
+
+What: /sys/class/genwqe/genwqen_card/type
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:Type of the card e.g. 'GenWQE5-A7'.
+
+What: /sys/class/genwqe/genwqen_card/cpld_version
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:Version id of the cards CPLD.
+
+What: /sys/class/genwqe/genwqen_card/curr_bitstream
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:Currently active bitstream. 1 is default, 0 is backup.
+
+What: /sys/class/genwqe/genwqen_card/next_bitstream
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:Interface to set the next bitstream to be used.
+
+What: /sys/class/genwqe/genwqen_card/tempsens
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:Interface to read the cards temperature sense register.
+
+What: /sys/class/genwqe/genwqen_card/state
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:State of the card: unused, used, error.
+
+What: /sys/class/genwqe/genwqen_card/base_clock
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:Base clock frequency of the card.
+
+What: /sys/class/genwqe/genwqen_card/device/sriov_numvfs
+Date: Oct 2013
+Contact:ha...@linux.vnet.ibm.com
+Description:Enable VFs (1..15):
+ sudo sh -c 'echo 15 \
+/sys/bus/pci/devices/\:1b\:00.0/sriov_numvfs'
+Disable VFs:
+ Write a 0 into the same sysfs entry.
diff --git a/drivers/misc/genwqe/card_sysfs.c
b/drivers/misc/genwqe/card_sysfs.c
new file mode 100644
index 000..f29ce55
--- /dev/null
+++ b/drivers/misc/genwqe/card_sysfs.c
@@ -0,0 +1,290 @@
+/**
+ * IBM Accelerator Family 'GenWQE'
+ *
+ * (C) Copyright IBM Corp. 2013
+ *
+ * Author: Frank Haverkamp ha...@linux.vnet.ibm.com
+ * Author: Joerg-Stephan Vogt jsv...@de.ibm.com
+ * Author: Michael Jung mij...@de.ibm.com
+ * Author: Michael Ruettger mich...@ibmra.de
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2 only)
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * Sysfs interfaces for the GenWQE card. There are attributes to query
+ * the version of the bitstream as well as some for the driver. For
+ * debugging, please also see the debugfs interfaces of this driver.
+ */
+
+#include linux/version.h
+#include linux/kernel.h
+#include linux/types.h
+#include linux/module.h
+#include linux/pci.h
+#include linux/string.h
+#include linux/fs.h
+#include linux/sysfs.h
+#include linux/ctype.h
+#include linux/device.h
+
+#include card_base.h
+#include card_ddcb.h
+
+static const char * const genwqe_types[] = {
+ [GENWQE_TYPE_ALTERA_230] = GenWQE4-230,
+ [GENWQE_TYPE_ALTERA_530] = GenWQE4-530,
+ [GENWQE_TYPE_ALTERA_A4] = GenWQE5-A4,
+ [GENWQE_TYPE_ALTERA_A7] =
Re: [PATCH 4/6] GenWQE Debugfs interfaces
On 05/11/13 19:44, Frank Haverkamp wrote: Debugfs interfaces for the GenWQE card. Help to debug potential problems. Dump internal chip state for debugging and failure determination. Signed-off-by: Frank Haverkamp ha...@linux.vnet.ibm.com Co-authors: Joerg-Stephan Vogt jsv...@de.ibm.com, Michael Jung mij...@de.ibm.com, Michael Ruettger mich...@ibmra.de Couple of comments below. ~Ryan --- Documentation/ABI/testing/debugfs-driver-genwqe | 70 +++ drivers/misc/genwqe/card_debugfs.c | 579 +++ 2 files changed, 649 insertions(+), 0 deletions(-) create mode 100644 Documentation/ABI/testing/debugfs-driver-genwqe create mode 100644 drivers/misc/genwqe/card_debugfs.c diff --git a/Documentation/ABI/testing/debugfs-driver-genwqe b/Documentation/ABI/testing/debugfs-driver-genwqe new file mode 100644 index 000..548883a --- /dev/null +++ b/Documentation/ABI/testing/debugfs-driver-genwqe @@ -0,0 +1,70 @@ +What: /sys/kernel/debug/genwqe/genwqen_card/ddcb_info +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:DDCB queue dump used for debugging queueing problems. + +What: /sys/kernel/debug/genwqe/genwqen_card/curr_regs +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Dump of the current error registers. + +What: /sys/kernel/debug/genwqe/genwqen_card/curr_uid0 +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Internal chip state of UID0 (unit id 0). + +What: /sys/kernel/debug/genwqe/genwqen_card/curr_uid1 +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Internal chip state of UID1. + +What: /sys/kernel/debug/genwqe/genwqen_card/curr_uid2 +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Internal chip state of UID2. + +What: /sys/kernel/debug/genwqe/genwqen_card/prev_regs +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Dump of the error registers before the last reset of +the card occured. + +What: /sys/kernel/debug/genwqe/genwqen_card/prev_uid0 +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Internal chip state of UID0 before card was reset. + +What: /sys/kernel/debug/genwqe/genwqen_card/prev_uid1 +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Internal chip state of UID1 before card was reset. + +What: /sys/kernel/debug/genwqe/genwqen_card/prev_uid2 +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Internal chip state of UID2 before card was reset. + +What: /sys/kernel/debug/genwqe/genwqen_card/info +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Comprehensive summary of bitstream version and software +version. Used bitstream and bitstream clocking information. + +What: /sys/kernel/debug/genwqe/genwqen_card/err_inject +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Possibility to inject error cases to ensure that the drivers +error handling code works well. + +What: /sys/kernel/debug/genwqe/genwqen_card/vf0..14_jobtimeout_msec +Date: Oct 2013 +Contact:ha...@linux.vnet.ibm.com +Description:Default VF timeout 250ms. Testing might require 1000ms. +Using 0 will use the cards default value (whatever that is). + +The timeout depends on the max number of available cards +in the system and the maximum allowed queue size. + +The driver ensures that the settings are done just before +the VFs get enabled. Changing the timeouts in flight is not +possible. diff --git a/drivers/misc/genwqe/card_debugfs.c b/drivers/misc/genwqe/card_debugfs.c new file mode 100644 index 000..ebf2f93 --- /dev/null +++ b/drivers/misc/genwqe/card_debugfs.c @@ -0,0 +1,579 @@ +/** + * IBM Accelerator Family 'GenWQE' + * + * (C) Copyright IBM Corp. 2013 + * + * Author: Frank Haverkamp ha...@linux.vnet.ibm.com + * Author: Joerg-Stephan Vogt jsv...@de.ibm.com + * Author: Michael Jung mij...@de.ibm.com + * Author: Michael Ruettger mich...@ibmra.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License (version 2 only) + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +
Re: [PATCH 2/9] GenWQE: Remove option to select name
On 05/11/13 04:08, Frank Haverkamp wrote: > Selecting interface names via configuration option is obsolete. Don't do this. You are adding completely new code, so there is no reason to post a patch full of code that is known to be incorrect, followed by a set of patches fixing things. Just post the correct code to start with. This makes the git history cleaner, and makes the code easier to review. You can use tools like git interactive rebase to split your work into multiple patches. It should be quite easy for this because you mostly just want to break down things to the file level, so even: git reset --soft and then manually staging and commiting things from there would probably be enough. You can use git add --interactive to stage individual file hunks if you want to break things down further. ~Ryan > > Signed-off-by: Frank Haverkamp > --- > drivers/misc/genwqe/Kconfig| 14 ++ > include/linux/genwqe/genwqe_card.h |6 +- > 2 files changed, 3 insertions(+), 17 deletions(-) > > diff --git a/drivers/misc/genwqe/Kconfig b/drivers/misc/genwqe/Kconfig > index bbf137d..6069d8c 100644 > --- a/drivers/misc/genwqe/Kconfig > +++ b/drivers/misc/genwqe/Kconfig > @@ -9,15 +9,5 @@ menuconfig GENWQE > default n > help > Enables PCIe card driver for IBM GenWQE accelerators. > - The user-space interface is described in > - include/linux/genwqe/genwqe_card.h. > - > -if GENWQE > - > -config GENWQE_DEVNAME > -string "Name for sysfs and device nodes" > - default "genwqe" > -help > - Select alternate name for sysfs and device nodes. > - > -endif > + The user-space interface is described in > + include/linux/genwqe/genwqe_card.h. > diff --git a/include/linux/genwqe/genwqe_card.h > b/include/linux/genwqe/genwqe_card.h > index 2c33db1..ffed142 100644 > --- a/include/linux/genwqe/genwqe_card.h > +++ b/include/linux/genwqe/genwqe_card.h > @@ -41,11 +41,7 @@ > #endif > > /* Basename of sysfs, debugfs and /dev interfaces */ > -#if defined(CONFIG_GENWQE_DEVNAME) > -# define GENWQE_DEVNAME CONFIG_GENWQE_DEVNAME > -#else > -# define GENWQE_DEVNAME "genwqe" > -#endif > +#define GENWQE_DEVNAME "genwqe" > > #define GENWQE_TYPE_ALTERA_230 0x00 /* GenWQE4 Stratix-IV-230 > */ > #define GENWQE_TYPE_ALTERA_530 0x01 /* GenWQE4 Stratix-IV-530 > */ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/9] GenWQE: Remove option to select name
On 05/11/13 04:08, Frank Haverkamp wrote: Selecting interface names via configuration option is obsolete. Don't do this. You are adding completely new code, so there is no reason to post a patch full of code that is known to be incorrect, followed by a set of patches fixing things. Just post the correct code to start with. This makes the git history cleaner, and makes the code easier to review. You can use tools like git interactive rebase to split your work into multiple patches. It should be quite easy for this because you mostly just want to break down things to the file level, so even: git reset --soft your-first-commit and then manually staging and commiting things from there would probably be enough. You can use git add --interactive to stage individual file hunks if you want to break things down further. ~Ryan Signed-off-by: Frank Haverkamp ha...@linux.vnet.ibm.com --- drivers/misc/genwqe/Kconfig| 14 ++ include/linux/genwqe/genwqe_card.h |6 +- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/drivers/misc/genwqe/Kconfig b/drivers/misc/genwqe/Kconfig index bbf137d..6069d8c 100644 --- a/drivers/misc/genwqe/Kconfig +++ b/drivers/misc/genwqe/Kconfig @@ -9,15 +9,5 @@ menuconfig GENWQE default n help Enables PCIe card driver for IBM GenWQE accelerators. - The user-space interface is described in - include/linux/genwqe/genwqe_card.h. - -if GENWQE - -config GENWQE_DEVNAME -string Name for sysfs and device nodes - default genwqe -help - Select alternate name for sysfs and device nodes. - -endif + The user-space interface is described in + include/linux/genwqe/genwqe_card.h. diff --git a/include/linux/genwqe/genwqe_card.h b/include/linux/genwqe/genwqe_card.h index 2c33db1..ffed142 100644 --- a/include/linux/genwqe/genwqe_card.h +++ b/include/linux/genwqe/genwqe_card.h @@ -41,11 +41,7 @@ #endif /* Basename of sysfs, debugfs and /dev interfaces */ -#if defined(CONFIG_GENWQE_DEVNAME) -# define GENWQE_DEVNAME CONFIG_GENWQE_DEVNAME -#else -# define GENWQE_DEVNAME genwqe -#endif +#define GENWQE_DEVNAME genwqe #define GENWQE_TYPE_ALTERA_230 0x00 /* GenWQE4 Stratix-IV-230 */ #define GENWQE_TYPE_ALTERA_530 0x01 /* GenWQE4 Stratix-IV-530 */ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On 30/10/13 20:32, Frank Haverkamp wrote:
> From: Frank Haverkamp
> Signed-off-by: Frank Haverkamp
> Co-authors: Joerg-Stephan Vogt ,
> Michael Jung ,
> Michael Ruettger
> ---
> Documentation/ABI/testing/debugfs-driver-genwqe | 57 +
> Documentation/ABI/testing/sysfs-driver-genwqe | 46 +
> drivers/misc/Kconfig|1 +
> drivers/misc/Makefile |1 +
> drivers/misc/genwqe/Kconfig | 23 +
> drivers/misc/genwqe/Makefile|8 +
> drivers/misc/genwqe/card_base.c | 1238 +++
> drivers/misc/genwqe/card_base.h | 569 +
> drivers/misc/genwqe/card_ddcb.c | 1380 +
> drivers/misc/genwqe/card_ddcb.h | 188 +++
> drivers/misc/genwqe/card_debugfs.c | 566 +
> drivers/misc/genwqe/card_dev.c | 1499
> +++
> drivers/misc/genwqe/card_sysfs.c| 306 +
> drivers/misc/genwqe/card_utils.c| 983 +++
> drivers/misc/genwqe/genwqe_driver.h | 75 ++
> include/linux/genwqe/genwqe_card.h | 559 +
> 16 files changed, 7499 insertions(+), 0 deletions(-)
Please consider breaking this into multiple patches. One behemoth patch
is harder to review and less likely for people to read all of it. Since
this adds a new driver, you can add it in parts, e.g: core, sysfs,
documentation, etc, and add the Makefile/Kconfig bits in the final
patch. That way it won't break the build at any point.
Couple of comments on the sysfs bits below.
~Ryan
> +++ b/drivers/misc/genwqe/card_sysfs.c
> @@ -0,0 +1,306 @@
> +/**
> + * IBM Accelerator Family 'GenWQE'
> + *
> + * (C) Copyright IBM Corp. 2013
> + *
> + * Author: Frank Haverkamp
> + * Author: Joerg-Stephan Vogt
> + * Author: Michael Jung
> + * Author: Michael Ruettger
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License (version 2 only)
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +/*
> + * Sysfs interfaces for the GenWQE card. There are attributes to query
> + * the version of the bitstream as well as some for the
> + * driver. Additionally there are some attributes which help to debug
> + * potential problems.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "card_base.h"
> +#include "card_ddcb.h"
> +
> +static const char * const genwqe_types[] = {
> + [GENWQE_TYPE_ALTERA_230] = "GenWQE4-230",
> + [GENWQE_TYPE_ALTERA_530] = "GenWQE4-530",
> + [GENWQE_TYPE_ALTERA_A4] = "GenWQE5-A4",
> + [GENWQE_TYPE_ALTERA_A7] = "GenWQE5-A7",
> +};
> +
> +static ssize_t show_card_status(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> + const char *cs[GENWQE_CARD_STATE_MAX] = { "unused", "used", "error" };
> +
> + len += scnprintf([len], PAGE_SIZE - len,
> + "%s\n", cs[cd->card_state]);
> + return len;
This is a bit confusingly written. Why do:
scnprintf([len], ...
When len is always zero at that point? Since you are printing from an
array of statically sized strings that are guaranteed to be smaller than
PAGE_SIZE, you can safely do:
sprintf(buf, "%s\n", cs[cd->card_state]);
You might want to add some checking for cd->card_state being out of
bounds, and printing "unknown" or something in that case.
Same issue exists in other sysfs handlers.
> +}
> +
> +static ssize_t show_card_appid(struct device *dev,
> +struct device_attribute *attr,
> +char *buf)
> +{
> + ssize_t len = 0;
> + char app_name[5];
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + genwqe_read_app_id(cd, app_name, sizeof(app_name));
> + len += scnprintf([len], PAGE_SIZE - len,
> + "%s\n", app_name);
> + return len;
> +}
> +
> +static ssize_t show_card_version(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + ssize_t len = 0;
> + u64 slu_id, app_id;
> + struct genwqe_dev *cd = dev_get_drvdata(dev);
> +
> + slu_id = __genwqe_readq(cd, IO_SLU_UNITCFG);
> + app_id = __genwqe_readq(cd, IO_APP_UNITCFG);
> +
> + len += scnprintf([len], PAGE_SIZE - len,
> +
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On 31/10/13 04:35, Greg KH wrote: > On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: >> +if GENWQE >> + >> +config GENWQE_DEVNAME >> +string "Name for sysfs and device nodes" >> +default "genwqe" >> +help >> + Select alternate name for sysfs and device nodes. > > Don't let the user pick this, it's up to the driver to set this once and > then live with it. > > And, from what I can tell in the driver, this help text is wrong (there > are no device nodes with this name in it, right?) The indentation is also messed up here. Kconfig should use hard tabs, with two additional spaces for the help text. This has mixed tabs and spaces between lines. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On 31/10/13 04:35, Greg KH wrote: On Wed, Oct 30, 2013 at 10:32:58AM +0100, Frank Haverkamp wrote: +if GENWQE + +config GENWQE_DEVNAME +string Name for sysfs and device nodes +default genwqe +help + Select alternate name for sysfs and device nodes. Don't let the user pick this, it's up to the driver to set this once and then live with it. And, from what I can tell in the driver, this help text is wrong (there are no device nodes with this name in it, right?) The indentation is also messed up here. Kconfig should use hard tabs, with two additional spaces for the help text. This has mixed tabs and spaces between lines. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Generic WorkQueue Engine (GenWQE) device driver (v4)
On 30/10/13 20:32, Frank Haverkamp wrote:
From: Frank Haverkamp ha...@vnet.ibm.com
Signed-off-by: Frank Haverkamp ha...@linux.vnet.ibm.com
Co-authors: Joerg-Stephan Vogt jsv...@de.ibm.com,
Michael Jung mij...@de.ibm.com,
Michael Ruettger mich...@ibmra.de
---
Documentation/ABI/testing/debugfs-driver-genwqe | 57 +
Documentation/ABI/testing/sysfs-driver-genwqe | 46 +
drivers/misc/Kconfig|1 +
drivers/misc/Makefile |1 +
drivers/misc/genwqe/Kconfig | 23 +
drivers/misc/genwqe/Makefile|8 +
drivers/misc/genwqe/card_base.c | 1238 +++
drivers/misc/genwqe/card_base.h | 569 +
drivers/misc/genwqe/card_ddcb.c | 1380 +
drivers/misc/genwqe/card_ddcb.h | 188 +++
drivers/misc/genwqe/card_debugfs.c | 566 +
drivers/misc/genwqe/card_dev.c | 1499
+++
drivers/misc/genwqe/card_sysfs.c| 306 +
drivers/misc/genwqe/card_utils.c| 983 +++
drivers/misc/genwqe/genwqe_driver.h | 75 ++
include/linux/genwqe/genwqe_card.h | 559 +
16 files changed, 7499 insertions(+), 0 deletions(-)
Please consider breaking this into multiple patches. One behemoth patch
is harder to review and less likely for people to read all of it. Since
this adds a new driver, you can add it in parts, e.g: core, sysfs,
documentation, etc, and add the Makefile/Kconfig bits in the final
patch. That way it won't break the build at any point.
Couple of comments on the sysfs bits below.
~Ryan
+++ b/drivers/misc/genwqe/card_sysfs.c
@@ -0,0 +1,306 @@
+/**
+ * IBM Accelerator Family 'GenWQE'
+ *
+ * (C) Copyright IBM Corp. 2013
+ *
+ * Author: Frank Haverkamp ha...@linux.vnet.ibm.com
+ * Author: Joerg-Stephan Vogt jsv...@de.ibm.com
+ * Author: Michael Jung mij...@de.ibm.com
+ * Author: Michael Ruettger mich...@ibmra.de
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2 only)
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * Sysfs interfaces for the GenWQE card. There are attributes to query
+ * the version of the bitstream as well as some for the
+ * driver. Additionally there are some attributes which help to debug
+ * potential problems.
+ */
+
+#include linux/version.h
+#include linux/kernel.h
+#include linux/types.h
+#include linux/module.h
+#include linux/pci.h
+#include linux/string.h
+#include linux/fs.h
+#include linux/sysfs.h
+#include linux/ctype.h
+
+#include card_base.h
+#include card_ddcb.h
+
+static const char * const genwqe_types[] = {
+ [GENWQE_TYPE_ALTERA_230] = GenWQE4-230,
+ [GENWQE_TYPE_ALTERA_530] = GenWQE4-530,
+ [GENWQE_TYPE_ALTERA_A4] = GenWQE5-A4,
+ [GENWQE_TYPE_ALTERA_A7] = GenWQE5-A7,
+};
+
+static ssize_t show_card_status(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ ssize_t len = 0;
+ struct genwqe_dev *cd = dev_get_drvdata(dev);
+ const char *cs[GENWQE_CARD_STATE_MAX] = { unused, used, error };
+
+ len += scnprintf(buf[len], PAGE_SIZE - len,
+ %s\n, cs[cd-card_state]);
+ return len;
This is a bit confusingly written. Why do:
scnprintf(buf[len], ...
When len is always zero at that point? Since you are printing from an
array of statically sized strings that are guaranteed to be smaller than
PAGE_SIZE, you can safely do:
sprintf(buf, %s\n, cs[cd-card_state]);
You might want to add some checking for cd-card_state being out of
bounds, and printing unknown or something in that case.
Same issue exists in other sysfs handlers.
+}
+
+static ssize_t show_card_appid(struct device *dev,
+struct device_attribute *attr,
+char *buf)
+{
+ ssize_t len = 0;
+ char app_name[5];
+ struct genwqe_dev *cd = dev_get_drvdata(dev);
+
+ genwqe_read_app_id(cd, app_name, sizeof(app_name));
+ len += scnprintf(buf[len], PAGE_SIZE - len,
+ %s\n, app_name);
+ return len;
+}
+
+static ssize_t show_card_version(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ ssize_t len = 0;
+ u64 slu_id, app_id;
+ struct genwqe_dev *cd = dev_get_drvdata(dev);
+
+ slu_id =
Re: [PATCH 1/2 v3] ARM: ep93xx_defconfig: cleanup ep93xx_defconfig
On 18/10/13 05:25, H Hartley Sweeten wrote: > Generate ep93xx_defconfig by doing: > > make ep93xx_defconfig > make savedefconfig > mv defconfig arch/arm/configs/ep93xx_defconfig > > No functional change. This just refreshes the ep93xx_defconfig to make it > easier and cleaner when adding new entries. > > Signed-off-by: H Hartley Sweeten Acked-by: Ryan Mallon > Cc: Ryan Mallon > Cc: Alan Stern > Cc: Lennert Buytenhek > Cc: Greg Kroah-Hartman > Cc: Olof Johansson > Cc: Russell King > --- > arch/arm/configs/ep93xx_defconfig | 16 ++-- > 1 file changed, 2 insertions(+), 14 deletions(-) > > diff --git a/arch/arm/configs/ep93xx_defconfig > b/arch/arm/configs/ep93xx_defconfig > index 806005a..8eccbcb 100644 > --- a/arch/arm/configs/ep93xx_defconfig > +++ b/arch/arm/configs/ep93xx_defconfig > @@ -1,15 +1,14 @@ > -CONFIG_EXPERIMENTAL=y > CONFIG_SYSVIPC=y > CONFIG_IKCONFIG=y > CONFIG_IKCONFIG_PROC=y > CONFIG_LOG_BUF_SHIFT=14 > -CONFIG_SYSFS_DEPRECATED_V2=y > CONFIG_EXPERT=y > CONFIG_SLAB=y > CONFIG_MODULES=y > CONFIG_MODULE_UNLOAD=y > CONFIG_MODULE_FORCE_UNLOAD=y > # CONFIG_BLK_DEV_BSG is not set > +CONFIG_PARTITION_ADVANCED=y > # CONFIG_IOSCHED_CFQ is not set > CONFIG_ARCH_EP93XX=y > CONFIG_CRUNCH=y > @@ -47,11 +46,8 @@ CONFIG_IPV6=y > CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug" > # CONFIG_FW_LOADER is not set > CONFIG_MTD=y > -CONFIG_MTD_CONCAT=y > -CONFIG_MTD_PARTITIONS=y > CONFIG_MTD_REDBOOT_PARTS=y > CONFIG_MTD_CMDLINE_PARTS=y > -CONFIG_MTD_CHAR=y > CONFIG_MTD_BLOCK=y > CONFIG_MTD_CFI=y > CONFIG_MTD_CFI_ADV_OPTIONS=y > @@ -67,15 +63,14 @@ CONFIG_SCSI=y > # CONFIG_SCSI_PROC_FS is not set > CONFIG_BLK_DEV_SD=y > CONFIG_NETDEVICES=y > -CONFIG_NET_ETHERNET=y > CONFIG_EP93XX_ETH=y > CONFIG_USB_RTL8150=y > # CONFIG_INPUT is not set > # CONFIG_SERIO is not set > # CONFIG_VT is not set > +# CONFIG_LEGACY_PTYS is not set > CONFIG_SERIAL_AMBA_PL010=y > CONFIG_SERIAL_AMBA_PL010_CONSOLE=y > -# CONFIG_LEGACY_PTYS is not set > # CONFIG_HW_RANDOM is not set > CONFIG_I2C=y > CONFIG_I2C_CHARDEV=y > @@ -86,7 +81,6 @@ CONFIG_WATCHDOG=y > CONFIG_EP93XX_WATCHDOG=y > CONFIG_USB=y > CONFIG_USB_DEBUG=y > -CONFIG_USB_DEVICEFS=y > CONFIG_USB_DYNAMIC_MINORS=y > CONFIG_USB_OHCI_HCD=y > CONFIG_USB_STORAGE=y > @@ -100,24 +94,18 @@ CONFIG_RTC_DRV_EP93XX=y > CONFIG_EXT2_FS=y > CONFIG_EXT3_FS=y > # CONFIG_EXT3_FS_XATTR is not set > -CONFIG_INOTIFY=y > CONFIG_VFAT_FS=y > CONFIG_TMPFS=y > CONFIG_JFFS2_FS=y > CONFIG_NFS_FS=y > -CONFIG_NFS_V3=y > CONFIG_ROOT_NFS=y > -CONFIG_PARTITION_ADVANCED=y > CONFIG_NLS_CODEPAGE_437=y > CONFIG_NLS_ISO8859_1=y > CONFIG_MAGIC_SYSRQ=y > -CONFIG_DEBUG_KERNEL=y > CONFIG_DEBUG_SLAB=y > CONFIG_DEBUG_SPINLOCK=y > CONFIG_DEBUG_MUTEXES=y > -# CONFIG_RCU_CPU_STALL_DETECTOR is not set > CONFIG_DEBUG_USER=y > -CONFIG_DEBUG_ERRORS=y > CONFIG_DEBUG_LL=y > # CONFIG_CRYPTO_ANSI_CPRNG is not set > CONFIG_LIBCRC32C=y -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] ARM: ep93xx_defconfig: cleanup ep93xx_defconfig
On 18/10/13 05:25, H Hartley Sweeten wrote: Generate ep93xx_defconfig by doing: make ep93xx_defconfig make savedefconfig mv defconfig arch/arm/configs/ep93xx_defconfig No functional change. This just refreshes the ep93xx_defconfig to make it easier and cleaner when adding new entries. Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com Acked-by: Ryan Mallon rmal...@gmail.com Cc: Ryan Mallon rmal...@gmail.com Cc: Alan Stern st...@rowland.harvard.edu Cc: Lennert Buytenhek ker...@wantstofly.org Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Olof Johansson o...@lixom.net Cc: Russell King li...@arm.linux.org.uk --- arch/arm/configs/ep93xx_defconfig | 16 ++-- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/arch/arm/configs/ep93xx_defconfig b/arch/arm/configs/ep93xx_defconfig index 806005a..8eccbcb 100644 --- a/arch/arm/configs/ep93xx_defconfig +++ b/arch/arm/configs/ep93xx_defconfig @@ -1,15 +1,14 @@ -CONFIG_EXPERIMENTAL=y CONFIG_SYSVIPC=y CONFIG_IKCONFIG=y CONFIG_IKCONFIG_PROC=y CONFIG_LOG_BUF_SHIFT=14 -CONFIG_SYSFS_DEPRECATED_V2=y CONFIG_EXPERT=y CONFIG_SLAB=y CONFIG_MODULES=y CONFIG_MODULE_UNLOAD=y CONFIG_MODULE_FORCE_UNLOAD=y # CONFIG_BLK_DEV_BSG is not set +CONFIG_PARTITION_ADVANCED=y # CONFIG_IOSCHED_CFQ is not set CONFIG_ARCH_EP93XX=y CONFIG_CRUNCH=y @@ -47,11 +46,8 @@ CONFIG_IPV6=y CONFIG_UEVENT_HELPER_PATH=/sbin/hotplug # CONFIG_FW_LOADER is not set CONFIG_MTD=y -CONFIG_MTD_CONCAT=y -CONFIG_MTD_PARTITIONS=y CONFIG_MTD_REDBOOT_PARTS=y CONFIG_MTD_CMDLINE_PARTS=y -CONFIG_MTD_CHAR=y CONFIG_MTD_BLOCK=y CONFIG_MTD_CFI=y CONFIG_MTD_CFI_ADV_OPTIONS=y @@ -67,15 +63,14 @@ CONFIG_SCSI=y # CONFIG_SCSI_PROC_FS is not set CONFIG_BLK_DEV_SD=y CONFIG_NETDEVICES=y -CONFIG_NET_ETHERNET=y CONFIG_EP93XX_ETH=y CONFIG_USB_RTL8150=y # CONFIG_INPUT is not set # CONFIG_SERIO is not set # CONFIG_VT is not set +# CONFIG_LEGACY_PTYS is not set CONFIG_SERIAL_AMBA_PL010=y CONFIG_SERIAL_AMBA_PL010_CONSOLE=y -# CONFIG_LEGACY_PTYS is not set # CONFIG_HW_RANDOM is not set CONFIG_I2C=y CONFIG_I2C_CHARDEV=y @@ -86,7 +81,6 @@ CONFIG_WATCHDOG=y CONFIG_EP93XX_WATCHDOG=y CONFIG_USB=y CONFIG_USB_DEBUG=y -CONFIG_USB_DEVICEFS=y CONFIG_USB_DYNAMIC_MINORS=y CONFIG_USB_OHCI_HCD=y CONFIG_USB_STORAGE=y @@ -100,24 +94,18 @@ CONFIG_RTC_DRV_EP93XX=y CONFIG_EXT2_FS=y CONFIG_EXT3_FS=y # CONFIG_EXT3_FS_XATTR is not set -CONFIG_INOTIFY=y CONFIG_VFAT_FS=y CONFIG_TMPFS=y CONFIG_JFFS2_FS=y CONFIG_NFS_FS=y -CONFIG_NFS_V3=y CONFIG_ROOT_NFS=y -CONFIG_PARTITION_ADVANCED=y CONFIG_NLS_CODEPAGE_437=y CONFIG_NLS_ISO8859_1=y CONFIG_MAGIC_SYSRQ=y -CONFIG_DEBUG_KERNEL=y CONFIG_DEBUG_SLAB=y CONFIG_DEBUG_SPINLOCK=y CONFIG_DEBUG_MUTEXES=y -# CONFIG_RCU_CPU_STALL_DETECTOR is not set CONFIG_DEBUG_USER=y -CONFIG_DEBUG_ERRORS=y CONFIG_DEBUG_LL=y # CONFIG_CRYPTO_ANSI_CPRNG is not set CONFIG_LIBCRC32C=y -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: ohci: remove ep93xx bus glue platform driver
On 15/10/13 08:35, H Hartley Sweeten wrote:
> Convert ep93xx to use the OHCI platform driver and remove the
> ohci-ep93xx bus glue driver.
>
> Signed-off-by: H Hartley Sweeten
> Cc: Alan Stern
> Cc: Greg Kroah-Hartman
> Cc: Ryan Mallon
Cc'ed Olof, who wanted to see what the patches looked like.
I think the overall code reduction is probably worth the small increase
in arch/arm code size.
~Ryan
> ---
> arch/arm/mach-ep93xx/clock.c | 2 +-
> arch/arm/mach-ep93xx/core.c| 45 +-
> drivers/usb/host/Kconfig | 2 +-
> drivers/usb/host/ohci-ep93xx.c | 184
> -
> drivers/usb/host/ohci-hcd.c| 18
> 5 files changed, 43 insertions(+), 208 deletions(-)
> delete mode 100644 drivers/usb/host/ohci-ep93xx.c
>
> diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
> index c95dbce..39ef3b6 100644
> --- a/arch/arm/mach-ep93xx/clock.c
> +++ b/arch/arm/mach-ep93xx/clock.c
> @@ -212,7 +212,7 @@ static struct clk_lookup clocks[] = {
> INIT_CK(NULL, "hclk", _h),
> INIT_CK(NULL, "apb_pclk", _p),
> INIT_CK(NULL, "pll2", _pll2),
> - INIT_CK("ep93xx-ohci", NULL, _usb_host),
> + INIT_CK("ohci-platform",NULL, _usb_host),
> INIT_CK("ep93xx-keypad",NULL, _keypad),
> INIT_CK("ep93xx-fb",NULL, _video),
> INIT_CK("ep93xx-spi.0", NULL, _spi),
> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> index 3f12b88..5489824 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -36,6 +36,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -297,22 +298,58 @@ static struct platform_device ep93xx_rtc_device = {
> .resource = ep93xx_rtc_resource,
> };
>
> +/*
> + * EP93xx OHCI USB Host
> + */
> +
> +static struct clk *ep93xx_ohci_host_clock;
> +
> +static int ep93xx_ohci_power_on(struct platform_device *pdev)
> +{
> + if (!ep93xx_ohci_host_clock) {
> + ep93xx_ohci_host_clock = devm_clk_get(>dev, NULL);
> + if (IS_ERR(ep93xx_ohci_host_clock))
> + return PTR_ERR(ep93xx_ohci_host_clock);
> + }
> +
> + clk_enable(ep93xx_ohci_host_clock);
> +
> + return 0;
> +}
> +
> +static void ep93xx_ohci_power_off(struct platform_device *pdev)
> +{
> + clk_disable(ep93xx_ohci_host_clock);
> +}
> +
> +static void ep93xx_ohci_power_suspend(struct platform_device *pdev)
> +{
> + ep93xx_ohci_power_off(pdev);
> +}
> +
> +static struct usb_ohci_pdata ep93xx_ohci_pdata = {
> + .power_on = ep93xx_ohci_power_on,
> + .power_off = ep93xx_ohci_power_off,
> + .power_suspend = ep93xx_ohci_power_suspend,
> +};
>
> static struct resource ep93xx_ohci_resources[] = {
> DEFINE_RES_MEM(EP93XX_USB_PHYS_BASE, 0x1000),
> DEFINE_RES_IRQ(IRQ_EP93XX_USB),
> };
>
> +static u64 ep93xx_ohci_dma_mask = DMA_BIT_MASK(32);
>
> static struct platform_device ep93xx_ohci_device = {
> - .name = "ep93xx-ohci",
> + .name = "ohci-platform",
> .id = -1,
> + .num_resources = ARRAY_SIZE(ep93xx_ohci_resources),
> + .resource = ep93xx_ohci_resources,
> .dev= {
> - .dma_mask =
> _ohci_device.dev.coherent_dma_mask,
> + .dma_mask = _ohci_dma_mask,
> .coherent_dma_mask = DMA_BIT_MASK(32),
> + .platform_data = _ohci_pdata,
> },
> - .num_resources = ARRAY_SIZE(ep93xx_ohci_resources),
> - .resource = ep93xx_ohci_resources,
> };
>
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index b3f20d7..2c8f2db 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -472,7 +472,7 @@ config USB_CNS3XXX_OHCI
>
> config USB_OHCI_HCD_PLATFORM
> tristate "Generic OHCI driver for a platform device"
> - default n
> + default y if ARCH_EP93XX
> ---help---
> Adds an OHCI host driver for a generic platform device, which
> provides a memory space and an irq.
> diff --git a/drivers/usb/host/ohci-ep93xx.c b/drivers/usb/host/ohci-ep93x
Re: [PATCH] ARM: ep93xx: remove deprecated IRQF_DISABLED
On 15/10/13 02:47, Hartley Sweeten wrote:
> On Friday, October 11, 2013 8:11 PM, Michael Opdenacker wrote:
>> This patch proposes to remove the use of the IRQF_DISABLED flag
>>
>> It's a NOOP since 2.6.35 and it will be removed one day.
>>
>> Signed-off-by: Michael Opdenacker
>> ---
>> arch/arm/mach-ep93xx/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
>> index 3f12b88..fcc965f 100644
>> --- a/arch/arm/mach-ep93xx/core.c
>> +++ b/arch/arm/mach-ep93xx/core.c
>> @@ -136,7 +136,7 @@ static irqreturn_t ep93xx_timer_interrupt(int irq, void
>> *dev_id)
>>
>> static struct irqaction ep93xx_timer_irq = {
>> .name = "ep93xx timer",
>> -.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
>> +.flags = IRQF_TIMER | IRQF_IRQPOLL,
>> .handler= ep93xx_timer_interrupt,
>> };
>>
>
> Acked-by: H Hartley Sweeten
Applied to ep93xx-fixes.
Thanks,
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4] vsprintf: Check real user/group id for %pK
Some setuid binaries will allow reading of files which have read permission by the real user id. This is problematic with files which use %pK because the file access permission is checked at open() time, but the kptr_restrict setting is checked at read() time. If a setuid binary opens a %pK file as an unprivileged user, and then elevates permissions before reading the file, then kernel pointer values may be leaked. This happens for example with the setuid pppd application on Ubuntu 12.04: $ head -1 /proc/kallsyms T startup_32 $ pppd file /proc/kallsyms pppd: In file /proc/kallsyms: unrecognized option 'c100' This will only leak the pointer value from the first line, but other setuid binaries may leak more information. Fix this by adding a check that in addition to the current process having CAP_SYSLOG, that effective user and group ids are equal to the real ids. If a setuid binary reads the contents of a file which uses %pK then the pointer values will be printed as NULL if the real user is unprivileged. Update the sysctl documentation to reflect the changes, and also correct the documentation to state the kptr_restrict=0 is the default. This is a only temporary solution to the issue. The correct solution is to do the permission check at open() time on files, and to replace %pK with a function which checks the open() time permission. %pK uses in printk should be removed since no sane permission check can be done, and instead protected by using dmesg_restrict. Signed-off-by: Ryan Mallon Cc: sta...@vger.kernel.org --- This is a temporary solution only, but fixes a minor security hole when kptr_restrict=1. I am working to fix this properly, but there is still some discussion around how to achieve this, see here: https://lkml.org/lkml/2013/10/10/805 This solution at least resolves the problem, and can easily be cherry picked into stable/distro kernels as needed. Changes since v3: * Update the sysctl documentation to detail the reasons for the check and also explain the temporary nature of the solution. * Added Cc stable, since this patch should probably be applied to stable kernels, and possibly distro kernels. * Cc'ed people for Documentation changes Changes since v2: * Fixed typo in comment: 'proccess' -> 'process' * Use a switch statement for the kptr_restrict values * Updated the sysctl documentation Changes since v1: * Fix the description to say 'vsprintf' instead of 'printk'. * Clarify the open() vs read() time checks in the patch description and code comment. * Remove comment about 'badly written' setuid binaries. This occurs with setuids binaries which correctly handle opening files. * Added extra people to the Cc list. --- diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt index 9d4c1d1..fb78e60 100644 --- a/Documentation/sysctl/kernel.txt +++ b/Documentation/sysctl/kernel.txt @@ -290,13 +290,24 @@ 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. +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. This is +because %pK checks are done at read() time rather than open() time, so +if permissions are elevated between the open() and the read() (e.g via +a setuid binary) then %pK will not leak kernel pointers to unprivileged +users. Note, this is a temporary solution only. The correct long-term +solution is to do the permission checks at open() time. Consider removing +world read permissions from files that use %pK, and using dmesg_restrict +to protect against uses of %pK in dmesg(8) if leaking kernel pointer +values to unprivileged users is a concern. + +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..d76555c 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -27,6 +27,7 @@ #include #include #include +#include #include #include /* for PAGE_SIZE */ @@ -1312,11 +1313,37 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, spec.field_width = default_width; retu
Re: [PATCH] usb: ohci: remove ep93xx bus glue platform driver
On 15/10/13 08:35, H Hartley Sweeten wrote: > Convert ep93xx to use the OHCI platform driver and remove the > ohci-ep93xx bus glue driver. > > Signed-off-by: H Hartley Sweeten > Cc: Alan Stern > Cc: Greg Kroah-Hartman > Cc: Ryan Mallon > --- > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index b3f20d7..2c8f2db 100644 > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -472,7 +472,7 @@ config USB_CNS3XXX_OHCI > > config USB_OHCI_HCD_PLATFORM > tristate "Generic OHCI driver for a platform device" > - default n > + default y if ARCH_EP93XX Shouldn't we select USB_OHCI_HCD_PLATFORM, e.g. something like: config ARCH_EP93XX_USB tristate "USB OHCI support" default y select USB_OHCI_HCD_PLATFORM In arch/arm/mach-ep93xx/Kconfig rather than polluting drivers/usb/host/Kconfig with arch specific stuff? ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 14/10/13 21:17, Djalal Harouni wrote: > On Fri, Oct 11, 2013 at 02:19:14PM +1100, Ryan Mallon wrote: >> On 11/10/13 13:20, Eric W. Biederman wrote: >>> Joe Perches writes: >>> >>>> Some setuid binaries will allow reading of files which have read >>>> permission by the real user id. This is problematic with files which >>>> use %pK because the file access permission is checked at open() time, >>>> but the kptr_restrict setting is checked at read() time. If a setuid >>>> binary opens a %pK file as an unprivileged user, and then elevates >>>> permissions before reading the file, then kernel pointer values may be >>>> leaked. >>>> >>>> This happens for example with the setuid pppd application on Ubuntu >>>> 12.04: >>>> >>>> $ head -1 /proc/kallsyms >>>> T startup_32 >>>> >>>> $ pppd file /proc/kallsyms >>>> pppd: In file /proc/kallsyms: unrecognized option 'c100' >>>> >>>> This will only leak the pointer value from the first line, but other >>>> setuid binaries may leak more information. >>>> >>>> Fix this by adding a check that in addition to the current process >>>> having CAP_SYSLOG, that effective user and group ids are equal to the >>>> real ids. If a setuid binary reads the contents of a file which uses >>>> %pK then the pointer values will be printed as NULL if the real user >>>> is unprivileged. >>>> >>>> Update the sysctl documentation to reflect the changes, and also >>>> correct the documentation to state the kptr_restrict=0 is the default. >>> >>> Sigh. This is all wrong. The only correct thing to test is >>> file->f_cred. Aka the capabilities of the program that opened the >>> file. >>> >>> Which means that the interface to %pK in the case of kptr_restrict is >>> broken as it has no way to be passed the information it needs to make >>> a sensible decision. >> >> Would it make sense to add a struct file * to struct seq_file and set >> that in seq_open? Then the capability check can be done against seq->file. > For the "add a struct file * to struct seq_file" and set it during > seq_open(), It was proposed by Linus, but Al Viro didn't like it: > https://lkml.org/lkml/2013/9/25/765 > > I'm not sure if this will work for you: you can make seq_file->private > cache some data, by calling single_open()... at ->open(), later check it > during read()... > > > As noted by Eric, I'll also go for the capability check at ->open(), if it > does not break some userspace. BTW the CAP_SYSLOG check should do the job Yes, it has already been agreed on that open() time is the correct place to do the check, and that either the check value should be cached in the struct seq_file or the struct cred/file should be kept so that the check can be done later. I think caching the result is actually better since it removes the whole in_irq() problem also. The problem I have at the moment is handling the case for /sys/module/sections/*, which are seq_files in Greg's driver-core tree, but do not actually pass the struct seq_file to the show function, which makes it impossible to check what the open() time permissions were. > Checks during read() are not sufficient, since the design allows passing > file descriptors and dup() stdin/stdout of suid-execve. > > > IMO: unprivileged code should not get that file descriptor, so ->open() > should fail. > If this will break userspace then allow open() and cache result for read() > > > Can you emulate the behaviour of kptr_restrict=1 ? If so: > 1) perform check during open() and cache data > 2) during read() check kptr_restrict==1 >check the cached value and if opener had CAP_SYSLOG if so: >print something like this: T startup_32 > > All this without modifying vsprintf, I mean just do the checks outside > vsprintf() inside your ->read() Again, this has already been agreed on. As suggested by George I have a function called seq_kernel_pointer(), which returns either the real pointer or NULL based on the cached check at seq_open(), so you do prints like: seq_printf(seq, "secret value = %p\n", seq_kernel_pointer(seq, ptr)); Once I figure out how to resolve the module sections case, I will post a patch series. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 12/10/13 09:37, Eric W. Biederman wrote: > Ryan Mallon writes: > >> The only remaining problem is kernel/module.c:module_sect_show() which >> is used to write the sysfs files in /sys/module//sections/. >> Those files are actually are really good target for leaking %pK values >> via setuid binaries. The problem is that the module_sect_show() function >> isn't passed information about who opened the sysfs file. I don't think >> this information is available in general for sysfs files either. Also, >> I can't actually see how module_sect_show() gets called? >> >> I'm a bit stuck on how to solve this. Any ideas? > > I haven't yet had a chance to review the patches but there are patches > to make sysfs files seq files in Greg's driver core tree. Hmm, I had a look at the driver-core tree, and although sysfs files internally use the seq_file interface, the sysfs show/store handlers do not get passed the struct seq_file, so doesn't appear possible to do the checks there. We could add a sysfs_ops->seq_show, but that feels clunky to have two different interfaces for handling sysfs files. Converting the whole tree to pass struct seq_file to the sysfs handlers would be painful :-/. I assume the reason the /sys/module//sections/* cannot be made 0400 is that some user-space tools are expecting those files to be readable by unprivileged users? ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 12/10/13 09:37, Eric W. Biederman wrote: Ryan Mallon rmal...@gmail.com writes: The only remaining problem is kernel/module.c:module_sect_show() which is used to write the sysfs files in /sys/module/modname/sections/. Those files are actually are really good target for leaking %pK values via setuid binaries. The problem is that the module_sect_show() function isn't passed information about who opened the sysfs file. I don't think this information is available in general for sysfs files either. Also, I can't actually see how module_sect_show() gets called? I'm a bit stuck on how to solve this. Any ideas? I haven't yet had a chance to review the patches but there are patches to make sysfs files seq files in Greg's driver core tree. Hmm, I had a look at the driver-core tree, and although sysfs files internally use the seq_file interface, the sysfs show/store handlers do not get passed the struct seq_file, so doesn't appear possible to do the checks there. We could add a sysfs_ops-seq_show, but that feels clunky to have two different interfaces for handling sysfs files. Converting the whole tree to pass struct seq_file to the sysfs handlers would be painful :-/. I assume the reason the /sys/module/modname/sections/* cannot be made 0400 is that some user-space tools are expecting those files to be readable by unprivileged users? ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 14/10/13 21:17, Djalal Harouni wrote:
On Fri, Oct 11, 2013 at 02:19:14PM +1100, Ryan Mallon wrote:
On 11/10/13 13:20, Eric W. Biederman wrote:
Joe Perches j...@perches.com writes:
Some setuid binaries will allow reading of files which have read
permission by the real user id. This is problematic with files which
use %pK because the file access permission is checked at open() time,
but the kptr_restrict setting is checked at read() time. If a setuid
binary opens a %pK file as an unprivileged user, and then elevates
permissions before reading the file, then kernel pointer values may be
leaked.
This happens for example with the setuid pppd application on Ubuntu
12.04:
$ head -1 /proc/kallsyms
T startup_32
$ pppd file /proc/kallsyms
pppd: In file /proc/kallsyms: unrecognized option 'c100'
This will only leak the pointer value from the first line, but other
setuid binaries may leak more information.
Fix this by adding a check that in addition to the current process
having CAP_SYSLOG, that effective user and group ids are equal to the
real ids. If a setuid binary reads the contents of a file which uses
%pK then the pointer values will be printed as NULL if the real user
is unprivileged.
Update the sysctl documentation to reflect the changes, and also
correct the documentation to state the kptr_restrict=0 is the default.
Sigh. This is all wrong. The only correct thing to test is
file-f_cred. Aka the capabilities of the program that opened the
file.
Which means that the interface to %pK in the case of kptr_restrict is
broken as it has no way to be passed the information it needs to make
a sensible decision.
Would it make sense to add a struct file * to struct seq_file and set
that in seq_open? Then the capability check can be done against seq-file.
For the add a struct file * to struct seq_file and set it during
seq_open(), It was proposed by Linus, but Al Viro didn't like it:
https://lkml.org/lkml/2013/9/25/765
I'm not sure if this will work for you: you can make seq_file-private
cache some data, by calling single_open()... at -open(), later check it
during read()...
As noted by Eric, I'll also go for the capability check at -open(), if it
does not break some userspace. BTW the CAP_SYSLOG check should do the job
Yes, it has already been agreed on that open() time is the correct place to
do the check, and that either the check value should be cached in the
struct seq_file or the struct cred/file should be kept so that the check
can be done later. I think caching the result is actually better since it
removes the whole in_irq() problem also.
The problem I have at the moment is handling the case for
/sys/modulemodname/sections/*, which are seq_files in Greg's
driver-core tree, but do not actually pass the struct seq_file to the show
function, which makes it impossible to check what the open() time
permissions were.
Checks during read() are not sufficient, since the design allows passing
file descriptors and dup() stdin/stdout of suid-execve.
IMO: unprivileged code should not get that file descriptor, so -open()
should fail.
If this will break userspace then allow open() and cache result for read()
Can you emulate the behaviour of kptr_restrict=1 ? If so:
1) perform check during open() and cache data
2) during read() check kptr_restrict==1
check the cached value and if opener had CAP_SYSLOG if so:
print something like this: T startup_32
All this without modifying vsprintf, I mean just do the checks outside
vsprintf() inside your -read()
Again, this has already been agreed on. As suggested by George I have
a function called seq_kernel_pointer(), which returns either the real
pointer or NULL based on the cached check at seq_open(), so you do
prints like:
seq_printf(seq, secret value = %p\n, seq_kernel_pointer(seq, ptr));
Once I figure out how to resolve the module sections case, I will post
a patch series.
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: ohci: remove ep93xx bus glue platform driver
On 15/10/13 08:35, H Hartley Sweeten wrote: Convert ep93xx to use the OHCI platform driver and remove the ohci-ep93xx bus glue driver. Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com Cc: Alan Stern st...@rowland.harvard.edu Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Ryan Mallon rmal...@gmail.com --- diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index b3f20d7..2c8f2db 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -472,7 +472,7 @@ config USB_CNS3XXX_OHCI config USB_OHCI_HCD_PLATFORM tristate Generic OHCI driver for a platform device - default n + default y if ARCH_EP93XX Shouldn't we select USB_OHCI_HCD_PLATFORM, e.g. something like: config ARCH_EP93XX_USB tristate USB OHCI support default y select USB_OHCI_HCD_PLATFORM In arch/arm/mach-ep93xx/Kconfig rather than polluting drivers/usb/host/Kconfig with arch specific stuff? ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4] vsprintf: Check real user/group id for %pK
Some setuid binaries will allow reading of files which have read permission by the real user id. This is problematic with files which use %pK because the file access permission is checked at open() time, but the kptr_restrict setting is checked at read() time. If a setuid binary opens a %pK file as an unprivileged user, and then elevates permissions before reading the file, then kernel pointer values may be leaked. This happens for example with the setuid pppd application on Ubuntu 12.04: $ head -1 /proc/kallsyms T startup_32 $ pppd file /proc/kallsyms pppd: In file /proc/kallsyms: unrecognized option 'c100' This will only leak the pointer value from the first line, but other setuid binaries may leak more information. Fix this by adding a check that in addition to the current process having CAP_SYSLOG, that effective user and group ids are equal to the real ids. If a setuid binary reads the contents of a file which uses %pK then the pointer values will be printed as NULL if the real user is unprivileged. Update the sysctl documentation to reflect the changes, and also correct the documentation to state the kptr_restrict=0 is the default. This is a only temporary solution to the issue. The correct solution is to do the permission check at open() time on files, and to replace %pK with a function which checks the open() time permission. %pK uses in printk should be removed since no sane permission check can be done, and instead protected by using dmesg_restrict. Signed-off-by: Ryan Mallon rmal...@gmail.com Cc: sta...@vger.kernel.org --- This is a temporary solution only, but fixes a minor security hole when kptr_restrict=1. I am working to fix this properly, but there is still some discussion around how to achieve this, see here: https://lkml.org/lkml/2013/10/10/805 This solution at least resolves the problem, and can easily be cherry picked into stable/distro kernels as needed. Changes since v3: * Update the sysctl documentation to detail the reasons for the check and also explain the temporary nature of the solution. * Added Cc stable, since this patch should probably be applied to stable kernels, and possibly distro kernels. * Cc'ed people for Documentation changes Changes since v2: * Fixed typo in comment: 'proccess' - 'process' * Use a switch statement for the kptr_restrict values * Updated the sysctl documentation Changes since v1: * Fix the description to say 'vsprintf' instead of 'printk'. * Clarify the open() vs read() time checks in the patch description and code comment. * Remove comment about 'badly written' setuid binaries. This occurs with setuids binaries which correctly handle opening files. * Added extra people to the Cc list. --- diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt index 9d4c1d1..fb78e60 100644 --- a/Documentation/sysctl/kernel.txt +++ b/Documentation/sysctl/kernel.txt @@ -290,13 +290,24 @@ 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. +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. This is +because %pK checks are done at read() time rather than open() time, so +if permissions are elevated between the open() and the read() (e.g via +a setuid binary) then %pK will not leak kernel pointers to unprivileged +users. Note, this is a temporary solution only. The correct long-term +solution is to do the permission checks at open() time. Consider removing +world read permissions from files that use %pK, and using dmesg_restrict +to protect against uses of %pK in dmesg(8) if leaking kernel pointer +values to unprivileged users is a concern. + +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..d76555c 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -27,6 +27,7 @@ #include linux/uaccess.h #include linux/ioport.h #include linux/dcache.h +#include linux/cred.h #include net/addrconf.h #include asm/page.h /* for PAGE_SIZE */ @@ -1312,11 +1313,37 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr
Re: [PATCH] ARM: ep93xx: remove deprecated IRQF_DISABLED
On 15/10/13 02:47, Hartley Sweeten wrote:
On Friday, October 11, 2013 8:11 PM, Michael Opdenacker wrote:
This patch proposes to remove the use of the IRQF_DISABLED flag
It's a NOOP since 2.6.35 and it will be removed one day.
Signed-off-by: Michael Opdenacker michael.opdenac...@free-electrons.com
---
arch/arm/mach-ep93xx/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
index 3f12b88..fcc965f 100644
--- a/arch/arm/mach-ep93xx/core.c
+++ b/arch/arm/mach-ep93xx/core.c
@@ -136,7 +136,7 @@ static irqreturn_t ep93xx_timer_interrupt(int irq, void
*dev_id)
static struct irqaction ep93xx_timer_irq = {
.name = ep93xx timer,
-.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
+.flags = IRQF_TIMER | IRQF_IRQPOLL,
.handler= ep93xx_timer_interrupt,
};
Acked-by: H Hartley Sweeten hswee...@visionengravers.com
Applied to ep93xx-fixes.
Thanks,
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: ohci: remove ep93xx bus glue platform driver
On 15/10/13 08:35, H Hartley Sweeten wrote:
Convert ep93xx to use the OHCI platform driver and remove the
ohci-ep93xx bus glue driver.
Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com
Cc: Alan Stern st...@rowland.harvard.edu
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Ryan Mallon rmal...@gmail.com
Cc'ed Olof, who wanted to see what the patches looked like.
I think the overall code reduction is probably worth the small increase
in arch/arm code size.
~Ryan
---
arch/arm/mach-ep93xx/clock.c | 2 +-
arch/arm/mach-ep93xx/core.c| 45 +-
drivers/usb/host/Kconfig | 2 +-
drivers/usb/host/ohci-ep93xx.c | 184
-
drivers/usb/host/ohci-hcd.c| 18
5 files changed, 43 insertions(+), 208 deletions(-)
delete mode 100644 drivers/usb/host/ohci-ep93xx.c
diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index c95dbce..39ef3b6 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -212,7 +212,7 @@ static struct clk_lookup clocks[] = {
INIT_CK(NULL, hclk, clk_h),
INIT_CK(NULL, apb_pclk, clk_p),
INIT_CK(NULL, pll2, clk_pll2),
- INIT_CK(ep93xx-ohci, NULL, clk_usb_host),
+ INIT_CK(ohci-platform,NULL, clk_usb_host),
INIT_CK(ep93xx-keypad,NULL, clk_keypad),
INIT_CK(ep93xx-fb,NULL, clk_video),
INIT_CK(ep93xx-spi.0, NULL, clk_spi),
diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
index 3f12b88..5489824 100644
--- a/arch/arm/mach-ep93xx/core.c
+++ b/arch/arm/mach-ep93xx/core.c
@@ -36,6 +36,7 @@
#include linux/export.h
#include linux/irqchip/arm-vic.h
#include linux/reboot.h
+#include linux/usb/ohci_pdriver.h
#include mach/hardware.h
#include linux/platform_data/video-ep93xx.h
@@ -297,22 +298,58 @@ static struct platform_device ep93xx_rtc_device = {
.resource = ep93xx_rtc_resource,
};
+/*
+ * EP93xx OHCI USB Host
+ */
+
+static struct clk *ep93xx_ohci_host_clock;
+
+static int ep93xx_ohci_power_on(struct platform_device *pdev)
+{
+ if (!ep93xx_ohci_host_clock) {
+ ep93xx_ohci_host_clock = devm_clk_get(pdev-dev, NULL);
+ if (IS_ERR(ep93xx_ohci_host_clock))
+ return PTR_ERR(ep93xx_ohci_host_clock);
+ }
+
+ clk_enable(ep93xx_ohci_host_clock);
+
+ return 0;
+}
+
+static void ep93xx_ohci_power_off(struct platform_device *pdev)
+{
+ clk_disable(ep93xx_ohci_host_clock);
+}
+
+static void ep93xx_ohci_power_suspend(struct platform_device *pdev)
+{
+ ep93xx_ohci_power_off(pdev);
+}
+
+static struct usb_ohci_pdata ep93xx_ohci_pdata = {
+ .power_on = ep93xx_ohci_power_on,
+ .power_off = ep93xx_ohci_power_off,
+ .power_suspend = ep93xx_ohci_power_suspend,
+};
static struct resource ep93xx_ohci_resources[] = {
DEFINE_RES_MEM(EP93XX_USB_PHYS_BASE, 0x1000),
DEFINE_RES_IRQ(IRQ_EP93XX_USB),
};
+static u64 ep93xx_ohci_dma_mask = DMA_BIT_MASK(32);
static struct platform_device ep93xx_ohci_device = {
- .name = ep93xx-ohci,
+ .name = ohci-platform,
.id = -1,
+ .num_resources = ARRAY_SIZE(ep93xx_ohci_resources),
+ .resource = ep93xx_ohci_resources,
.dev= {
- .dma_mask =
ep93xx_ohci_device.dev.coherent_dma_mask,
+ .dma_mask = ep93xx_ohci_dma_mask,
.coherent_dma_mask = DMA_BIT_MASK(32),
+ .platform_data = ep93xx_ohci_pdata,
},
- .num_resources = ARRAY_SIZE(ep93xx_ohci_resources),
- .resource = ep93xx_ohci_resources,
};
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index b3f20d7..2c8f2db 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -472,7 +472,7 @@ config USB_CNS3XXX_OHCI
config USB_OHCI_HCD_PLATFORM
tristate Generic OHCI driver for a platform device
- default n
+ default y if ARCH_EP93XX
---help---
Adds an OHCI host driver for a generic platform device, which
provides a memory space and an irq.
diff --git a/drivers/usb/host/ohci-ep93xx.c b/drivers/usb/host/ohci-ep93xx.c
deleted file mode 100644
index 84a20d5..000
--- a/drivers/usb/host/ohci-ep93xx.c
+++ /dev/null
@@ -1,184 +0,0 @@
-/*
- * OHCI HCD (Host Controller Driver) for USB.
- *
- * (C) Copyright 1999 Roman Weissgaerber wei...@vienna.at
- * (C) Copyright 2000-2002 David Brownell dbrown...@users.sourceforge.net
- * (C) Copyright 2002
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 11/10/13 15:42, George Spelvin wrote: > ebied...@xmission.com (Eric W. Biederman) wrote: >> Sigh. This is all wrong. The only correct thing to test is >> file->f_cred. Aka the capabilities of the program that opened the >> file. >> >> Which means that the interface to %pK in the case of kptr_restrict is >> broken as it has no way to be passed the information it needs to make >> a sensible decision. > > I looked at the code, and pretty painful. Certainly it's possible to > include a reference to the file (I was thinking of just the credentials, > actually) in the seq_file. But getting that to the vsprintf.c code > (specifically, the pointer() function) is a PITA. > > I'm willing to accept the currently proposed kludge as a "good enough" > approximation, as long as we're all agreed that using the credentials > at open() time would be The Right Thing, and hopefully someone will find > the round tuitts to implement that in future. > > But in the meantime, "the perfect is the enemey of the good" is worth > remembering. > > (An alternate implementation I've been thinking about would be to do > away with %pK, and instead have a "secret_ptr(p, seq->cred)" helper that > returned p or 0 depending on the credential.) I've been looking at this approach. The majority of %pK uses are in seq_files, so George's suggestion will work fine there. There are a handful of instances in printk() statements. I don't think %pK can ever make sense from printk(), because it is basically impossible to do any sane permission check. If a setuid binary does some action with elevated which causes printk() to be called then the user might see leaked kernel pointers. The correct way to prevent this is to use dmesg_restrict and not allow normal users to see the syslog. The only remaining problem is kernel/module.c:module_sect_show() which is used to write the sysfs files in /sys/module//sections/. Those files are actually are really good target for leaking %pK values via setuid binaries. The problem is that the module_sect_show() function isn't passed information about who opened the sysfs file. I don't think this information is available in general for sysfs files either. Also, I can't actually see how module_sect_show() gets called? I'm a bit stuck on how to solve this. Any ideas? ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 11/10/13 15:42, George Spelvin wrote: ebied...@xmission.com (Eric W. Biederman) wrote: Sigh. This is all wrong. The only correct thing to test is file-f_cred. Aka the capabilities of the program that opened the file. Which means that the interface to %pK in the case of kptr_restrict is broken as it has no way to be passed the information it needs to make a sensible decision. I looked at the code, and pretty painful. Certainly it's possible to include a reference to the file (I was thinking of just the credentials, actually) in the seq_file. But getting that to the vsprintf.c code (specifically, the pointer() function) is a PITA. I'm willing to accept the currently proposed kludge as a good enough approximation, as long as we're all agreed that using the credentials at open() time would be The Right Thing, and hopefully someone will find the round tuitts to implement that in future. But in the meantime, the perfect is the enemey of the good is worth remembering. (An alternate implementation I've been thinking about would be to do away with %pK, and instead have a secret_ptr(p, seq-cred) helper that returned p or 0 depending on the credential.) I've been looking at this approach. The majority of %pK uses are in seq_files, so George's suggestion will work fine there. There are a handful of instances in printk() statements. I don't think %pK can ever make sense from printk(), because it is basically impossible to do any sane permission check. If a setuid binary does some action with elevated which causes printk() to be called then the user might see leaked kernel pointers. The correct way to prevent this is to use dmesg_restrict and not allow normal users to see the syslog. The only remaining problem is kernel/module.c:module_sect_show() which is used to write the sysfs files in /sys/module/modname/sections/. Those files are actually are really good target for leaking %pK values via setuid binaries. The problem is that the module_sect_show() function isn't passed information about who opened the sysfs file. I don't think this information is available in general for sysfs files either. Also, I can't actually see how module_sect_show() gets called? I'm a bit stuck on how to solve this. Any ideas? ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages
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 1454238ce lib/vsprintf.o.new
> 14568 6 0 1457438ee lib/vsprintf.o.old
>
> Signed-off-by: Joe Perches
> ---
> 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;
>
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 11/10/13 15:42, George Spelvin wrote: > ebied...@xmission.com (Eric W. Biederman) wrote: >> Sigh. This is all wrong. The only correct thing to test is >> file->f_cred. Aka the capabilities of the program that opened the >> file. >> >> Which means that the interface to %pK in the case of kptr_restrict is >> broken as it has no way to be passed the information it needs to make >> a sensible decision. > > I looked at the code, and pretty painful. Certainly it's possible to > include a reference to the file (I was thinking of just the credentials, > actually) in the seq_file. But getting that to the vsprintf.c code > (specifically, the pointer() function) is a PITA. > > I'm willing to accept the currently proposed kludge as a "good enough" > approximation, as long as we're all agreed that using the credentials > at open() time would be The Right Thing, and hopefully someone will find > the round tuitts to implement that in future. > > But in the meantime, "the perfect is the enemey of the good" is worth > remembering. > > (An alternate implementation I've been thinking about would be to do > away with %pK, and instead have a "secret_ptr(p, seq->cred)" helper that > returned p or 0 depending on the credential.) Yeah, that is probably the best solution. I'll try to put together a patch series doing this. It will obviously be more involved though, so I think it is still worth merging the original patch in the interm. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 11/10/13 13:20, Eric W. Biederman wrote: > Joe Perches writes: > >> Some setuid binaries will allow reading of files which have read >> permission by the real user id. This is problematic with files which >> use %pK because the file access permission is checked at open() time, >> but the kptr_restrict setting is checked at read() time. If a setuid >> binary opens a %pK file as an unprivileged user, and then elevates >> permissions before reading the file, then kernel pointer values may be >> leaked. >> >> This happens for example with the setuid pppd application on Ubuntu >> 12.04: >> >> $ head -1 /proc/kallsyms >> T startup_32 >> >> $ pppd file /proc/kallsyms >> pppd: In file /proc/kallsyms: unrecognized option 'c100' >> >> This will only leak the pointer value from the first line, but other >> setuid binaries may leak more information. >> >> Fix this by adding a check that in addition to the current process >> having CAP_SYSLOG, that effective user and group ids are equal to the >> real ids. If a setuid binary reads the contents of a file which uses >> %pK then the pointer values will be printed as NULL if the real user >> is unprivileged. >> >> Update the sysctl documentation to reflect the changes, and also >> correct the documentation to state the kptr_restrict=0 is the default. > > Sigh. This is all wrong. The only correct thing to test is > file->f_cred. Aka the capabilities of the program that opened the > file. > > Which means that the interface to %pK in the case of kptr_restrict is > broken as it has no way to be passed the information it needs to make > a sensible decision. Would it make sense to add a struct file * to struct seq_file and set that in seq_open? Then the capability check can be done against seq->file. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 11/10/13 13:20, Eric W. Biederman wrote:
Joe Perches j...@perches.com writes:
Some setuid binaries will allow reading of files which have read
permission by the real user id. This is problematic with files which
use %pK because the file access permission is checked at open() time,
but the kptr_restrict setting is checked at read() time. If a setuid
binary opens a %pK file as an unprivileged user, and then elevates
permissions before reading the file, then kernel pointer values may be
leaked.
This happens for example with the setuid pppd application on Ubuntu
12.04:
$ head -1 /proc/kallsyms
T startup_32
$ pppd file /proc/kallsyms
pppd: In file /proc/kallsyms: unrecognized option 'c100'
This will only leak the pointer value from the first line, but other
setuid binaries may leak more information.
Fix this by adding a check that in addition to the current process
having CAP_SYSLOG, that effective user and group ids are equal to the
real ids. If a setuid binary reads the contents of a file which uses
%pK then the pointer values will be printed as NULL if the real user
is unprivileged.
Update the sysctl documentation to reflect the changes, and also
correct the documentation to state the kptr_restrict=0 is the default.
Sigh. This is all wrong. The only correct thing to test is
file-f_cred. Aka the capabilities of the program that opened the
file.
Which means that the interface to %pK in the case of kptr_restrict is
broken as it has no way to be passed the information it needs to make
a sensible decision.
Would it make sense to add a struct file * to struct seq_file and set
that in seq_open? Then the capability check can be done against seq-file.
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 11/10/13 15:42, George Spelvin wrote: ebied...@xmission.com (Eric W. Biederman) wrote: Sigh. This is all wrong. The only correct thing to test is file-f_cred. Aka the capabilities of the program that opened the file. Which means that the interface to %pK in the case of kptr_restrict is broken as it has no way to be passed the information it needs to make a sensible decision. I looked at the code, and pretty painful. Certainly it's possible to include a reference to the file (I was thinking of just the credentials, actually) in the seq_file. But getting that to the vsprintf.c code (specifically, the pointer() function) is a PITA. I'm willing to accept the currently proposed kludge as a good enough approximation, as long as we're all agreed that using the credentials at open() time would be The Right Thing, and hopefully someone will find the round tuitts to implement that in future. But in the meantime, the perfect is the enemey of the good is worth remembering. (An alternate implementation I've been thinking about would be to do away with %pK, and instead have a secret_ptr(p, seq-cred) helper that returned p or 0 depending on the credential.) Yeah, that is probably the best solution. I'll try to put together a patch series doing this. It will obviously be more involved though, so I think it is still worth merging the original patch in the interm. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages
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 1454238ce lib/vsprintf.o.new
14568 6 0 1457438ee lib/vsprintf.o.old
Signed-off-by: Joe Perches j...@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
Re: [PATCH 1/2] vsprintf/sysctl: Bugfix, neaten and document %pK usages
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c designware make SCL and SDA falling time configurable
On 09/10/13 18:55, Mika Westerberg wrote: > On Tue, Oct 08, 2013 at 05:00:54PM +0200, Romain Baeriswyl wrote: >> static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable) >> @@ -286,6 +287,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev) >> u32 input_clock_khz; >> u32 hcnt, lcnt; >> u32 reg; >> +u32 sda_falling_time, scl_falling_time; >> >> input_clock_khz = dev->get_clk_rate_khz(dev); >> >> @@ -307,15 +309,25 @@ int i2c_dw_init(struct dw_i2c_dev *dev) >> >> /* set standard and fast speed deviders for high/low periods */ >> >> +if (dev->sda_falling_time) >> +sda_falling_time = dev->sda_falling_time; >> +else >> +sda_falling_time = 300; /* ns */ > > I think this looks better: > > sda_falling_time = dev->sda_falling_time ? dev->sda_falling_time : 300; You can also use the gcc-ism, which is a bit more concise: sda_falling_time = dev->sda_falling_time ?: 300; ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 10/10/13 10:09, Joe Perches wrote:
> Changes in V3a:
>
> Do the in_irq tests only when kptr_restrict is 1.
> Document the %pK mechanism in vsnprintf
> Add missing documentation for %pV and %pNF too
I really did mean post a follow-up/separate patch, not a different
version of mine. The missing documentation for %pV and %pNF fix is fine,
but does not belong in this patch. The kptr_restrict pK-error is a
separate issue, my patch deliberately did not touch it because it is
unrelated. If you want to change it, please do so in a seperate patch.
You also removed my comment explaining why the uid/gid check is
necessary :-/.
~Ryan
> Documentation/sysctl/kernel.txt | 17
> lib/vsprintf.c | 43
> -
> 2 files changed, 39 insertions(+), 21 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..3efcf29 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include /* for PAGE_SIZE */
> @@ -1301,21 +1302,34 @@ 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);
> + case 'K': /* see: Documentation/sysctl/kernel.txt */
> + switch (kptr_restrict) {
> + case 0: /* None (default) */
> + break;
> + case 1: { /* Restricted */
> + const struct cred *cred;
> +
> + 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);
> + }
> + cred = current_cred();
> + if (!has_capability_noaudit(current, CAP_SYSLOG) ||
> + !uid_eq(cred->euid, cred->uid) ||
> + !gid_eq(cred->egid, cred->gid))
> + ptr = NULL;
> + break;
> }
> - if (!((kptr_restrict == 0) ||
> - (kptr_restrict == 1 &&
> -has_capability_noaudit(current, CAP_SYSLOG
> + case 2: /* Never - Always emit 0 */
> + default:
> ptr = NULL;
> + break;
> + }
> break;
> case 'N':
> switch (fmt[1]) {
> @@ -1574,6 +1588,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
Re: [PATCH v3] vsprintf: Check real user/group id for %pK
On 10/10/13 09:33, Joe Perches wrote:
> On Thu, 2013-10-10 at 09:25 +1100, Ryan Mallon wrote:
>
>> if (kptr_restrict && (in_irq() || in_serving_softirq() ||
>>in_nmi())) {
>>
>> Is making sure that you don't have kernel code doing something like this:
>>
>> irqreturn_t some_irq_handler(int irq, void *data)
>> {
>> struct seq_file *seq = to_seq(data);
>>
>> seq_printf(seq, "value = %pK\n");
>> return IRQ_HANDLED;
>> }
>>
>> Because that obviously won't work when kptr_restrict=1 (because the
>> CAP_SYSLOG check is meaningless). However, the code is broken regardless
>> of the kptr_restrict value.
>
> The only brokenness I see here is that the code doesn't pass
> a pointer along with %pK
>
> seq_printf(seq, "value of seq: %pK\n", seq);
>
>> Since the default value of kptr_restrict is
>> 0, this kind of bug can go over-looked because the seq file will print
>> the pointer value correctly when kptr_restrict=0, and it will correctly
>> print 0's when kptr_restrict=2, but it will print 'pK-error' when
>> kptr_restrict=1. Doing the check in all cases makes it more likely that
>> bugs like this get found. In fact, doing something like:
>>
>> if (WARN_ON(in_irq() || in_serving_softirq() || in_nmi())) {
>>
>> Might be better, since that will print a stack-trace showing where the
>> offending vsprintf is.
>
> WARN_ON would be potentially _very_ noisy.
> Maybe a long period (once a day?) ratelimited dump_stack();
If it was noisy, it would indicate a bunch of broken kernel code which
needs fixing :-). Anyway, this is really a separate issue to what I am
trying to fix, which is why I left the original code intact. If you want
to change it, post a follow-up patch.
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] vsprintf: Check real user/group id for %pK
On 10/10/13 09:14, Joe Perches wrote:
> On Thu, 2013-10-10 at 09:04 +1100, Ryan Mallon wrote:
>> On 10/10/13 09:00, Joe Perches wrote:
> []
>>> Move the interrupt tests and pK-error printk
>>> into case 1:
>>>
>>> It's the only case where CAP_SYSLOG needs to be
>>> tested so it doesn't need to be above the switch.
>>
>> Like I said, I think it is useful to do the pK-error check anyway. It is
>> checking for internal kernel bugs, since if 'pK-error' ever gets
>> printed, then some kernel code is doing the wrong thing.
>
> I think you don't quite understand how kptr_restrict works.
>
> If it's 0, then the ptr value is always emitted naturally.
> if it's 2, then the ptr value is always emitted as 0.
I understand this.
>
>> Therefore, I
>> think it is useful to print it always (I would argue it even makes sense
>> when kptr_restrict=0).
>
> How? Maybe it's me that doesn't quite understand.
This check:
if (kptr_restrict && (in_irq() || in_serving_softirq() ||
in_nmi())) {
Is making sure that you don't have kernel code doing something like this:
irqreturn_t some_irq_handler(int irq, void *data)
{
struct seq_file *seq = to_seq(data);
seq_printf(seq, "value = %pK\n");
return IRQ_HANDLED;
}
Because that obviously won't work when kptr_restrict=1 (because the
CAP_SYSLOG check is meaningless). However, the code is broken regardless
of the kptr_restrict value. Since the default value of kptr_restrict is
0, this kind of bug can go over-looked because the seq file will print
the pointer value correctly when kptr_restrict=0, and it will correctly
print 0's when kptr_restrict=2, but it will print 'pK-error' when
kptr_restrict=1. Doing the check in all cases makes it more likely that
bugs like this get found. In fact, doing something like:
if (WARN_ON(in_irq() || in_serving_softirq() || in_nmi())) {
Might be better, since that will print a stack-trace showing where the
offending vsprintf is.
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] vsprintf: Check real user/group id for %pK
On 10/10/13 09:00, Joe Perches wrote: > On Thu, 2013-10-10 at 08:52 +1100, Ryan Mallon wrote: >> Some setuid binaries will allow reading of files which have read >> permission by the real user id. This is problematic with files which >> use %pK because the file access permission is checked at open() time, >> but the kptr_restrict setting is checked at read() time. If a setuid >> binary opens a %pK file as an unprivileged user, and then elevates >> permissions before reading the file, then kernel pointer values may be >> leaked. > > Please review the patch I sent you a little more. > >> Fix this by adding a check that in addition to the current process >> having CAP_SYSLOG, that effective user and group ids are equal to the >> real ids. If a setuid binary reads the contents of a file which uses >> %pK then the pointer values will be printed as NULL if the real user >> is unprivileged. > > [] > >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c > [] >> @@ -1312,11 +1313,37 @@ char *pointer(const char *fmt, char *buf, char *end, >> void *ptr, >> spec.field_width = default_width; >> return string(buf, end, "pK-error", spec); >> } > > Move the interrupt tests and pK-error printk > into case 1: > > It's the only case where CAP_SYSLOG needs to be > tested so it doesn't need to be above the switch. Like I said, I think it is useful to do the pK-error check anyway. It is checking for internal kernel bugs, since if 'pK-error' ever gets printed, then some kernel code is doing the wrong thing. Therefore, I think it is useful to print it always (I would argue it even makes sense when kptr_restrict=0). I decided to just leave that part of the code alone. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] vsprintf: Check real user/group id for %pK
Some setuid binaries will allow reading of files which have read
permission by the real user id. This is problematic with files which
use %pK because the file access permission is checked at open() time,
but the kptr_restrict setting is checked at read() time. If a setuid
binary opens a %pK file as an unprivileged user, and then elevates
permissions before reading the file, then kernel pointer values may be
leaked.
This happens for example with the setuid pppd application on Ubuntu 12.04:
$ head -1 /proc/kallsyms
T startup_32
$ pppd file /proc/kallsyms
pppd: In file /proc/kallsyms: unrecognized option 'c100'
This will only leak the pointer value from the first line, but other
setuid binaries may leak more information.
Fix this by adding a check that in addition to the current process
having CAP_SYSLOG, that effective user and group ids are equal to the
real ids. If a setuid binary reads the contents of a file which uses
%pK then the pointer values will be printed as NULL if the real user
is unprivileged.
Update the sysctl documentation to reflect the changes, and also
correct the documentation to state the kptr_restrict=0 is the default.
Signed-off-by: Ryan Mallon
---
Changes since v2:
* Fixed typo in comment: 'proccess' -> 'process'
* Use a switch statement for the kptr_restrict values
* Updated the sysctl documentation
Changes since v1:
* Fix the description to say 'vsprintf' instead of 'printk'.
* Clarify the open() vs read() time checks in the patch description
and code comment.
* Remove comment about 'badly written' setuid binaries. This occurs
with setuids binaries which correctly handle opening files.
* Added extra people to the Cc list.
---
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 9d4c1d1..d57766e 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -290,13 +290,14 @@ 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.
+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..d76555c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include /* for PAGE_SIZE */
@@ -1312,11 +1313,37 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
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
+
+ switch (kptr_restrict) {
+ case 0:
+ /* Always print %pK values */
+ break;
+ case 1: {
+ /*
+* Only print the real pointer value if the current
+* process has CAP_SYSLOG and is running with the
+* same credentials it started with. This is because
+* access to files is checked at open() time, but %pK
+* checks permission at read() time. We don't want to
+* leak pointer values if a binary opens a file using
+* %pK and then elevates privileges before reading it.
+*/
+ const struct cred *cred = current_cred();
+
+ if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+ !uid_eq(cred->euid, cred->uid) ||
+ !gid_eq(cred->egid, cred->gid))
+ ptr = NULL;
+ break;
+ }
+ case 2:
+ default:
+ /* Always print 0's for %pK */
ptr = NULL;
+ break;
+
[PATCH v3] vsprintf: Check real user/group id for %pK
Some setuid binaries will allow reading of files which have read
permission by the real user id. This is problematic with files which
use %pK because the file access permission is checked at open() time,
but the kptr_restrict setting is checked at read() time. If a setuid
binary opens a %pK file as an unprivileged user, and then elevates
permissions before reading the file, then kernel pointer values may be
leaked.
This happens for example with the setuid pppd application on Ubuntu 12.04:
$ head -1 /proc/kallsyms
T startup_32
$ pppd file /proc/kallsyms
pppd: In file /proc/kallsyms: unrecognized option 'c100'
This will only leak the pointer value from the first line, but other
setuid binaries may leak more information.
Fix this by adding a check that in addition to the current process
having CAP_SYSLOG, that effective user and group ids are equal to the
real ids. If a setuid binary reads the contents of a file which uses
%pK then the pointer values will be printed as NULL if the real user
is unprivileged.
Update the sysctl documentation to reflect the changes, and also
correct the documentation to state the kptr_restrict=0 is the default.
Signed-off-by: Ryan Mallon rmal...@gmail.com
---
Changes since v2:
* Fixed typo in comment: 'proccess' - 'process'
* Use a switch statement for the kptr_restrict values
* Updated the sysctl documentation
Changes since v1:
* Fix the description to say 'vsprintf' instead of 'printk'.
* Clarify the open() vs read() time checks in the patch description
and code comment.
* Remove comment about 'badly written' setuid binaries. This occurs
with setuids binaries which correctly handle opening files.
* Added extra people to the Cc list.
---
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 9d4c1d1..d57766e 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -290,13 +290,14 @@ 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.
+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..d76555c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@
#include linux/uaccess.h
#include linux/ioport.h
#include linux/dcache.h
+#include linux/cred.h
#include net/addrconf.h
#include asm/page.h /* for PAGE_SIZE */
@@ -1312,11 +1313,37 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
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
+
+ switch (kptr_restrict) {
+ case 0:
+ /* Always print %pK values */
+ break;
+ case 1: {
+ /*
+* Only print the real pointer value if the current
+* process has CAP_SYSLOG and is running with the
+* same credentials it started with. This is because
+* access to files is checked at open() time, but %pK
+* checks permission at read() time. We don't want to
+* leak pointer values if a binary opens a file using
+* %pK and then elevates privileges before reading it.
+*/
+ const struct cred *cred = current_cred();
+
+ if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+ !uid_eq(cred-euid, cred-uid) ||
+ !gid_eq(cred-egid, cred-gid))
+ ptr = NULL;
+ break;
+ }
+ case 2:
+ default:
+ /* Always print 0's for %pK */
ptr = NULL
Re: [PATCH v3] vsprintf: Check real user/group id for %pK
On 10/10/13 09:00, Joe Perches wrote: On Thu, 2013-10-10 at 08:52 +1100, Ryan Mallon wrote: Some setuid binaries will allow reading of files which have read permission by the real user id. This is problematic with files which use %pK because the file access permission is checked at open() time, but the kptr_restrict setting is checked at read() time. If a setuid binary opens a %pK file as an unprivileged user, and then elevates permissions before reading the file, then kernel pointer values may be leaked. Please review the patch I sent you a little more. Fix this by adding a check that in addition to the current process having CAP_SYSLOG, that effective user and group ids are equal to the real ids. If a setuid binary reads the contents of a file which uses %pK then the pointer values will be printed as NULL if the real user is unprivileged. [] diff --git a/lib/vsprintf.c b/lib/vsprintf.c [] @@ -1312,11 +1313,37 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, spec.field_width = default_width; return string(buf, end, pK-error, spec); } Move the interrupt tests and pK-error printk into case 1: It's the only case where CAP_SYSLOG needs to be tested so it doesn't need to be above the switch. Like I said, I think it is useful to do the pK-error check anyway. It is checking for internal kernel bugs, since if 'pK-error' ever gets printed, then some kernel code is doing the wrong thing. Therefore, I think it is useful to print it always (I would argue it even makes sense when kptr_restrict=0). I decided to just leave that part of the code alone. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] vsprintf: Check real user/group id for %pK
On 10/10/13 09:14, Joe Perches wrote:
On Thu, 2013-10-10 at 09:04 +1100, Ryan Mallon wrote:
On 10/10/13 09:00, Joe Perches wrote:
[]
Move the interrupt tests and pK-error printk
into case 1:
It's the only case where CAP_SYSLOG needs to be
tested so it doesn't need to be above the switch.
Like I said, I think it is useful to do the pK-error check anyway. It is
checking for internal kernel bugs, since if 'pK-error' ever gets
printed, then some kernel code is doing the wrong thing.
I think you don't quite understand how kptr_restrict works.
If it's 0, then the ptr value is always emitted naturally.
if it's 2, then the ptr value is always emitted as 0.
I understand this.
Therefore, I
think it is useful to print it always (I would argue it even makes sense
when kptr_restrict=0).
How? Maybe it's me that doesn't quite understand.
This check:
if (kptr_restrict (in_irq() || in_serving_softirq() ||
in_nmi())) {
Is making sure that you don't have kernel code doing something like this:
irqreturn_t some_irq_handler(int irq, void *data)
{
struct seq_file *seq = to_seq(data);
seq_printf(seq, value = %pK\n);
return IRQ_HANDLED;
}
Because that obviously won't work when kptr_restrict=1 (because the
CAP_SYSLOG check is meaningless). However, the code is broken regardless
of the kptr_restrict value. Since the default value of kptr_restrict is
0, this kind of bug can go over-looked because the seq file will print
the pointer value correctly when kptr_restrict=0, and it will correctly
print 0's when kptr_restrict=2, but it will print 'pK-error' when
kptr_restrict=1. Doing the check in all cases makes it more likely that
bugs like this get found. In fact, doing something like:
if (WARN_ON(in_irq() || in_serving_softirq() || in_nmi())) {
Might be better, since that will print a stack-trace showing where the
offending vsprintf is.
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] vsprintf: Check real user/group id for %pK
On 10/10/13 09:33, Joe Perches wrote:
On Thu, 2013-10-10 at 09:25 +1100, Ryan Mallon wrote:
if (kptr_restrict (in_irq() || in_serving_softirq() ||
in_nmi())) {
Is making sure that you don't have kernel code doing something like this:
irqreturn_t some_irq_handler(int irq, void *data)
{
struct seq_file *seq = to_seq(data);
seq_printf(seq, value = %pK\n);
return IRQ_HANDLED;
}
Because that obviously won't work when kptr_restrict=1 (because the
CAP_SYSLOG check is meaningless). However, the code is broken regardless
of the kptr_restrict value.
The only brokenness I see here is that the code doesn't pass
a pointer along with %pK
seq_printf(seq, value of seq: %pK\n, seq);
Since the default value of kptr_restrict is
0, this kind of bug can go over-looked because the seq file will print
the pointer value correctly when kptr_restrict=0, and it will correctly
print 0's when kptr_restrict=2, but it will print 'pK-error' when
kptr_restrict=1. Doing the check in all cases makes it more likely that
bugs like this get found. In fact, doing something like:
if (WARN_ON(in_irq() || in_serving_softirq() || in_nmi())) {
Might be better, since that will print a stack-trace showing where the
offending vsprintf is.
WARN_ON would be potentially _very_ noisy.
Maybe a long period (once a day?) ratelimited dump_stack();
If it was noisy, it would indicate a bunch of broken kernel code which
needs fixing :-). Anyway, this is really a separate issue to what I am
trying to fix, which is why I left the original code intact. If you want
to change it, post a follow-up patch.
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3a] vsprintf: Check real user/group id for %pK
On 10/10/13 10:09, Joe Perches wrote:
Changes in V3a:
Do the in_irq tests only when kptr_restrict is 1.
Document the %pK mechanism in vsnprintf
Add missing documentation for %pV and %pNF too
I really did mean post a follow-up/separate patch, not a different
version of mine. The missing documentation for %pV and %pNF fix is fine,
but does not belong in this patch. The kptr_restrict pK-error is a
separate issue, my patch deliberately did not touch it because it is
unrelated. If you want to change it, please do so in a seperate patch.
You also removed my comment explaining why the uid/gid check is
necessary :-/.
~Ryan
Documentation/sysctl/kernel.txt | 17
lib/vsprintf.c | 43
-
2 files changed, 39 insertions(+), 21 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..3efcf29 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@
#include linux/uaccess.h
#include linux/ioport.h
#include linux/dcache.h
+#include linux/cred.h
#include net/addrconf.h
#include asm/page.h/* for PAGE_SIZE */
@@ -1301,21 +1302,34 @@ 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);
+ case 'K': /* see: Documentation/sysctl/kernel.txt */
+ switch (kptr_restrict) {
+ case 0: /* None (default) */
+ break;
+ case 1: { /* Restricted */
+ const struct cred *cred;
+
+ 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);
+ }
+ cred = current_cred();
+ if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+ !uid_eq(cred-euid, cred-uid) ||
+ !gid_eq(cred-egid, cred-gid))
+ ptr = NULL;
+ break;
}
- if (!((kptr_restrict == 0) ||
- (kptr_restrict == 1
-has_capability_noaudit(current, CAP_SYSLOG
+ case 2: /* Never - Always emit 0 */
+ default:
ptr = NULL;
+ break;
+ }
break;
case 'N':
switch (fmt[1]) {
@@ -1574,6 +1588,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,
Re: [PATCH 1/2] i2c designware make SCL and SDA falling time configurable
On 09/10/13 18:55, Mika Westerberg wrote: On Tue, Oct 08, 2013 at 05:00:54PM +0200, Romain Baeriswyl wrote: static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable) @@ -286,6 +287,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev) u32 input_clock_khz; u32 hcnt, lcnt; u32 reg; +u32 sda_falling_time, scl_falling_time; input_clock_khz = dev-get_clk_rate_khz(dev); @@ -307,15 +309,25 @@ int i2c_dw_init(struct dw_i2c_dev *dev) /* set standard and fast speed deviders for high/low periods */ +if (dev-sda_falling_time) +sda_falling_time = dev-sda_falling_time; +else +sda_falling_time = 300; /* ns */ I think this looks better: sda_falling_time = dev-sda_falling_time ? dev-sda_falling_time : 300; You can also use the gcc-ism, which is a bit more concise: sda_falling_time = dev-sda_falling_time ?: 300; ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] vsprintf: Check real user/group id for %pK
On 09/10/13 13:00, Joe Perches wrote: > On Wed, 2013-10-09 at 12:55 +1100, Ryan Mallon wrote: >> On 09/10/13 12:30, Joe Perches wrote: >>> On Tue, 2013-10-08 at 17:49 -0700, Joe Perches wrote: >>>> On Wed, 2013-10-09 at 11:15 +1100, Ryan Mallon wrote: >>>>> Some setuid binaries will allow reading of files which have read >>>>> permission by the real user id. This is problematic with files which >>>>> use %pK because the file access permission is checked at open() time, >>>>> but the kptr_restrict setting is checked at read() time. If a setuid >>>>> binary opens a %pK file as an unprivileged user, and then elevates >>>>> permissions before reading the file, then kernel pointer values may be >>>>> leaked. >>>> I think it should explicitly test 0. >>> Also, Documentation/sysctl/kernel.txt should be updated too. >>> >>> Here's a suggested patch: >>> >>> --- >>> Documentation/sysctl/kernel.txt | 14 -- >>> lib/vsprintf.c | 38 ++ >>> 2 files changed, 34 insertions(+), 18 deletions(-) >>> >>> diff --git a/Documentation/sysctl/kernel.txt >>> b/Documentation/sysctl/kernel.txt >>> index 9d4c1d1..eac53d5 100644 >>> --- a/Documentation/sysctl/kernel.txt >>> +++ b/Documentation/sysctl/kernel.txt >>> @@ -290,13 +290,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 >>> +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. >>> +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. >> I'll add this, thanks. >> >> I'm less fussed about the suggestions for the logic. The current test is >> small and concise. > The logic ends up the same to the compiler, but it's > human readers that want simple and clear. > >> The original also does the in_irq tests regardless of >> the kptr_restrict setting since they are mostly intended to catch >> internal kernel bugs. > Not so. > > http://marc.info/?l=linux-security-module=129303800912245=4 > https://lkml.org/lkml/2012/7/13/428 > Ah, I misread it. It does however check when kptr_restrict != 0, not just when kptr_restrict is 1. I've left the in_irq test as-is, but used a switch as suggested. I don't really care either way, I think the original check is quite readable. Anyway, updated patch below: ~Ryan --- diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt index 9d4c1d1..eac53d5 100644 --- a/Documentation/sysctl/kernel.txt +++ b/Documentation/sysctl/kernel.txt @@ -290,13 +290,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 +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. +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..6dd8c5d 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -27,6 +27,7 @@ #include #include #include +#include #include #include
Re: [PATCH v2] vsprintf: Check real user/group id for %pK
On 09/10/13 12:30, Joe Perches wrote:
> On Tue, 2013-10-08 at 17:49 -0700, Joe Perches wrote:
>> On Wed, 2013-10-09 at 11:15 +1100, Ryan Mallon wrote:
>>> Some setuid binaries will allow reading of files which have read
>>> permission by the real user id. This is problematic with files which
>>> use %pK because the file access permission is checked at open() time,
>>> but the kptr_restrict setting is checked at read() time. If a setuid
>>> binary opens a %pK file as an unprivileged user, and then elevates
>>> permissions before reading the file, then kernel pointer values may be
>>> leaked.
>>
>> I think it should explicitly test 0.
>
> Also, Documentation/sysctl/kernel.txt should be updated too.
>
> Here's a suggested patch:
>
> ---
> Documentation/sysctl/kernel.txt | 14 --
> lib/vsprintf.c | 38 ++
> 2 files changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 9d4c1d1..eac53d5 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -290,13 +290,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
> +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.
> +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.
I'll add this, thanks.
I'm less fussed about the suggestions for the logic. The current test is
small and concise. The original also does the in_irq tests regardless of
the kptr_restrict setting since they are mostly intended to catch
internal kernel bugs.
Anyway, I am mostly interested to hear if the solution is acceptable, or
if a more involved open() vs read() time check is required.
~Ryan
>
> ==
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 26559bd..986fdbe 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include /* for PAGE_SIZE */
> @@ -1302,20 +1303,33 @@ 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: /* None */
> + break;
> + case 1: { /* Restricted (the default) */
> + const struct cred *cred;
> +
> + 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);
> + }
> + cred = current_cred();
> + if (!has_capability_noaudit(current, CAP_SYSLOG) ||
> + !uid_eq(cred->euid, cred->uid) ||
> + !gid_eq(cred->egid, cred->gid))
> + ptr = NULL;
> + break;
>
[PATCH v2] vsprintf: Check real user/group id for %pK
Some setuid binaries will allow reading of files which have read
permission by the real user id. This is problematic with files which
use %pK because the file access permission is checked at open() time,
but the kptr_restrict setting is checked at read() time. If a setuid
binary opens a %pK file as an unprivileged user, and then elevates
permissions before reading the file, then kernel pointer values may be
leaked.
This happens for example with the setuid pppd application on Ubuntu 12.04:
$ head -1 /proc/kallsyms
T startup_32
$ pppd file /proc/kallsyms
pppd: In file /proc/kallsyms: unrecognized option 'c100'
This will only leak the pointer value from the first line, but other
setuid binaries may leak more information.
Fix this by adding a check that in addition to the current process
having CAP_SYSLOG, that effective user and group ids are equal to the
real ids. If a setuid binary reads the contents of a file which uses
%pK then the pointer values will be printed as NULL if the real user
is unprivileged.
Signed-off-by: Ryan Mallon
---
Changes since v1:
* Fix the description to say 'vsprintf' instead of 'printk'.
* Clarify the open() vs read() time checks in the patch description and
code comment.
* Remove comment about 'badly written' setuid binaries. This occurs
with setuids binaries which correctly handle opening files.
* Added extra people to the Cc list.
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..c02faf3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1312,10 +1312,26 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
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
- ptr = NULL;
+
+ /*
+* If kptr_restrict is set to 2, then %pK always prints as
+* NULL. If it is set to 1, then only print the real pointer
+* value if the current proccess has CAP_SYSLOG and is running
+* with the same credentials it started with. This is because
+* access to files is checked at open() time, but %pK checks
+* permission at read() time. We don't want to leak pointer
+* values if a binary opens a file using %pK and then elevates
+* privileges before reading it.
+*/
+ {
+ const struct cred *cred = current_cred();
+
+ if (kptr_restrict == 2 || (kptr_restrict == 1 &&
+(!has_capability_noaudit(current, CAP_SYSLOG) ||
+ !uid_eq(cred->euid, cred->uid) ||
+ !gid_eq(cred->egid, cred->gid
+ ptr = NULL;
+ }
break;
case 'N':
switch (fmt[1]) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] vsprintf: Check real user/group id for %pK
Some setuid binaries will allow reading of files which have read
permission by the real user id. This is problematic with files which
use %pK because the file access permission is checked at open() time,
but the kptr_restrict setting is checked at read() time. If a setuid
binary opens a %pK file as an unprivileged user, and then elevates
permissions before reading the file, then kernel pointer values may be
leaked.
This happens for example with the setuid pppd application on Ubuntu 12.04:
$ head -1 /proc/kallsyms
T startup_32
$ pppd file /proc/kallsyms
pppd: In file /proc/kallsyms: unrecognized option 'c100'
This will only leak the pointer value from the first line, but other
setuid binaries may leak more information.
Fix this by adding a check that in addition to the current process
having CAP_SYSLOG, that effective user and group ids are equal to the
real ids. If a setuid binary reads the contents of a file which uses
%pK then the pointer values will be printed as NULL if the real user
is unprivileged.
Signed-off-by: Ryan Mallon rmal...@gmail.com
---
Changes since v1:
* Fix the description to say 'vsprintf' instead of 'printk'.
* Clarify the open() vs read() time checks in the patch description and
code comment.
* Remove comment about 'badly written' setuid binaries. This occurs
with setuids binaries which correctly handle opening files.
* Added extra people to the Cc list.
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..c02faf3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1312,10 +1312,26 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
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
- ptr = NULL;
+
+ /*
+* If kptr_restrict is set to 2, then %pK always prints as
+* NULL. If it is set to 1, then only print the real pointer
+* value if the current proccess has CAP_SYSLOG and is running
+* with the same credentials it started with. This is because
+* access to files is checked at open() time, but %pK checks
+* permission at read() time. We don't want to leak pointer
+* values if a binary opens a file using %pK and then elevates
+* privileges before reading it.
+*/
+ {
+ const struct cred *cred = current_cred();
+
+ if (kptr_restrict == 2 || (kptr_restrict == 1
+(!has_capability_noaudit(current, CAP_SYSLOG) ||
+ !uid_eq(cred-euid, cred-uid) ||
+ !gid_eq(cred-egid, cred-gid
+ ptr = NULL;
+ }
break;
case 'N':
switch (fmt[1]) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] vsprintf: Check real user/group id for %pK
On 09/10/13 12:30, Joe Perches wrote:
On Tue, 2013-10-08 at 17:49 -0700, Joe Perches wrote:
On Wed, 2013-10-09 at 11:15 +1100, Ryan Mallon wrote:
Some setuid binaries will allow reading of files which have read
permission by the real user id. This is problematic with files which
use %pK because the file access permission is checked at open() time,
but the kptr_restrict setting is checked at read() time. If a setuid
binary opens a %pK file as an unprivileged user, and then elevates
permissions before reading the file, then kernel pointer values may be
leaked.
I think it should explicitly test 0.
Also, Documentation/sysctl/kernel.txt should be updated too.
Here's a suggested patch:
---
Documentation/sysctl/kernel.txt | 14 --
lib/vsprintf.c | 38 ++
2 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 9d4c1d1..eac53d5 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -290,13 +290,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
+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.
+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.
I'll add this, thanks.
I'm less fussed about the suggestions for the logic. The current test is
small and concise. The original also does the in_irq tests regardless of
the kptr_restrict setting since they are mostly intended to catch
internal kernel bugs.
Anyway, I am mostly interested to hear if the solution is acceptable, or
if a more involved open() vs read() time check is required.
~Ryan
==
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..986fdbe 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@
#include linux/uaccess.h
#include linux/ioport.h
#include linux/dcache.h
+#include linux/cred.h
#include net/addrconf.h
#include asm/page.h/* for PAGE_SIZE */
@@ -1302,20 +1303,33 @@ 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: /* None */
+ break;
+ case 1: { /* Restricted (the default) */
+ const struct cred *cred;
+
+ 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);
+ }
+ cred = current_cred();
+ if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+ !uid_eq(cred-euid, cred-uid) ||
+ !gid_eq(cred-egid, cred-gid))
+ ptr = NULL;
+ break;
}
- if (!((kptr_restrict == 0) ||
- (kptr_restrict == 1
-has_capability_noaudit(current, CAP_SYSLOG
+ case 2: /* Forbidden - Always 0 */
+ default:
ptr = NULL;
+ break;
+ }
break;
case 'N':
switch (fmt[1]) {
--
To unsubscribe from
Re: [PATCH v2] vsprintf: Check real user/group id for %pK
On 09/10/13 13:00, Joe Perches wrote:
On Wed, 2013-10-09 at 12:55 +1100, Ryan Mallon wrote:
On 09/10/13 12:30, Joe Perches wrote:
On Tue, 2013-10-08 at 17:49 -0700, Joe Perches wrote:
On Wed, 2013-10-09 at 11:15 +1100, Ryan Mallon wrote:
Some setuid binaries will allow reading of files which have read
permission by the real user id. This is problematic with files which
use %pK because the file access permission is checked at open() time,
but the kptr_restrict setting is checked at read() time. If a setuid
binary opens a %pK file as an unprivileged user, and then elevates
permissions before reading the file, then kernel pointer values may be
leaked.
I think it should explicitly test 0.
Also, Documentation/sysctl/kernel.txt should be updated too.
Here's a suggested patch:
---
Documentation/sysctl/kernel.txt | 14 --
lib/vsprintf.c | 38 ++
2 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/Documentation/sysctl/kernel.txt
b/Documentation/sysctl/kernel.txt
index 9d4c1d1..eac53d5 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -290,13 +290,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
+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.
+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.
I'll add this, thanks.
I'm less fussed about the suggestions for the logic. The current test is
small and concise.
The logic ends up the same to the compiler, but it's
human readers that want simple and clear.
The original also does the in_irq tests regardless of
the kptr_restrict setting since they are mostly intended to catch
internal kernel bugs.
Not so.
http://marc.info/?l=linux-security-modulem=129303800912245w=4
https://lkml.org/lkml/2012/7/13/428
Ah, I misread it. It does however check when kptr_restrict != 0, not
just when kptr_restrict is 1. I've left the in_irq test as-is, but used
a switch as suggested. I don't really care either way, I think the
original check is quite readable. Anyway, updated patch below:
~Ryan
---
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 9d4c1d1..eac53d5 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -290,13 +290,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
+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.
+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..6dd8c5d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@
#include linux/uaccess.h
#include linux/ioport.h
#include linux/dcache.h
+#include linux/cred.h
#include net/addrconf.h
#include asm/page.h /* for PAGE_SIZE */
@@ -1312,11 +1313,36 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
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
+
+ switch (kptr_restrict) {
+ case 0:
+ /* Always print %pK values */
+ break;
+ case 1
Re: [PATCH 02/19] Make vma_dump_size() generic
On 04/10/13 20:30, Janani Venkataraman wrote:
> From:Suzuki K. Poulose
>
> vma_dump_size calculates the file size of a vma area in the core file. It
> assumes the vma belongs to the "current". Make it generic to work for any
> task.
> This will be reused by application core dump infrastructure.
>
> Signed-off-by: Suzuki K. Poulose
> ---
> -static unsigned long vma_dump_size(struct vm_area_struct *vma,
> +unsigned long vma_dump_size(struct task_struct *p, struct vm_area_struct
> *vma,
> unsigned long mm_flags)
> {
> #define FILTER(type)(mm_flags & (1UL << MMF_DUMP_##type))
> @@ -143,10 +143,18 @@ static unsigned long vma_dump_size(struct
> vm_area_struct *vma,
>* Switch to the user "segment" for get_user(),
>* then put back what elf_core_dump() had in place.
>*/
> - set_fs(USER_DS);
> - if (unlikely(get_user(word, header)))
> - word = 0;
> - set_fs(fs);
> + if (p->mm == current->mm) {
> + mm_segment_t fs = get_fs();
It looks like you missed the removal of the old:
mm_segment_t fs = get_fs();
above? Just below if (FILTER(ELF_HEADERS)).
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 02/19] Make vma_dump_size() generic
On 04/10/13 20:30, Janani Venkataraman wrote:
From:Suzuki K. Poulose suz...@in.ibm.com
vma_dump_size calculates the file size of a vma area in the core file. It
assumes the vma belongs to the current. Make it generic to work for any
task.
This will be reused by application core dump infrastructure.
Signed-off-by: Suzuki K. Poulose suz...@in.ibm.com
---
-static unsigned long vma_dump_size(struct vm_area_struct *vma,
+unsigned long vma_dump_size(struct task_struct *p, struct vm_area_struct
*vma,
unsigned long mm_flags)
{
#define FILTER(type)(mm_flags (1UL MMF_DUMP_##type))
@@ -143,10 +143,18 @@ static unsigned long vma_dump_size(struct
vm_area_struct *vma,
* Switch to the user segment for get_user(),
* then put back what elf_core_dump() had in place.
*/
- set_fs(USER_DS);
- if (unlikely(get_user(word, header)))
- word = 0;
- set_fs(fs);
+ if (p-mm == current-mm) {
+ mm_segment_t fs = get_fs();
It looks like you missed the removal of the old:
mm_segment_t fs = get_fs();
above? Just below if (FILTER(ELF_HEADERS)).
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}
On 04/10/13 10:41, Kees Cook wrote: > On Wed, Aug 28, 2013 at 1:49 PM, Kees Cook wrote: > > BTW, this just came to my attention: > http://marc.info/?l=linux-kernel=138049414321387=2 > > Same problem, just for /proc/kallsyms. This would benefit from the > open vs read cred check as well, I think. I was actually just about to put together a repost of this. Sorry I missed you off the original Cc list, get_maintainer didn't list you. I wanted to at least change the comment mentioning "badly written" setuid binaries. That isn't really true, as George Spelvin pointed out, even a setuid binary which opens the file with dropped priviledges, but reads it after re-elevating privileges will be susceptible to this. Setuid apps could be more precautious by doing the open + read into memory of user files with the privileges dropped, so that once privileges are re-elevated only the in-memory copy is used. I still think in-kernel fixing is a good idea too though, since it hardens against user-space setuid apps that don't do this. This was just the simplest approach to fixing the problem that I could think of. I'm open to suggestions for a better solution. ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] procfs: restore 0400 permissions on /proc/*/{syscall,stack,personality}
On 04/10/13 10:41, Kees Cook wrote: On Wed, Aug 28, 2013 at 1:49 PM, Kees Cook keesc...@chromium.org wrote: snip BTW, this just came to my attention: http://marc.info/?l=linux-kernelm=138049414321387w=2 Same problem, just for /proc/kallsyms. This would benefit from the open vs read cred check as well, I think. I was actually just about to put together a repost of this. Sorry I missed you off the original Cc list, get_maintainer didn't list you. I wanted to at least change the comment mentioning badly written setuid binaries. That isn't really true, as George Spelvin pointed out, even a setuid binary which opens the file with dropped priviledges, but reads it after re-elevating privileges will be susceptible to this. Setuid apps could be more precautious by doing the open + read into memory of user files with the privileges dropped, so that once privileges are re-elevated only the in-memory copy is used. I still think in-kernel fixing is a good idea too though, since it hardens against user-space setuid apps that don't do this. This was just the simplest approach to fixing the problem that I could think of. I'm open to suggestions for a better solution. ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging : android: binder.c: Prefer seq_puts to seq_printf
On 02/10/13 11:57, Mathieu Rhéaume wrote:
> This patch changes seq_printf for seq_puts in binder.c.
> It fixes the warnings emitted by checkpatch.pl.
>
> Signed-off-by: Mathieu Rhéaume
> ---
> drivers/staging/android/binder.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c
> b/drivers/staging/android/binder.c
> index 09edebb..f3d4a1f 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -3272,7 +3272,7 @@ binder_defer_work(struct binder_proc *proc, enum
> binder_deferred_state defer)
> static void print_binder_transaction(struct seq_file *m, const char *prefix,
>struct binder_transaction *t)
> {
> - seq_printf(m,
> + seq_puts(m,
> "%s %d: %p from %d:%d to %d:%d code %x flags %x pri %ld r%d",
> prefix, t->debug_id, t,
> t->from ? t->from->proc->pid : 0,
Umm, this won't compile. seq_puts() is defined as:
int seq_puts(struct seq_file *m, const char *s);
checkpatch.pl does indeed complain about uses of seq_printf() in
drivers/staging/android/binder.c which are passing formatted strings. At
a quick glance it looks like checkpatch.pl is only checking for the
format string on the same line as seq_printf(), so this files ugly
coding style confuses the check. Joe?
Anyway, please don't blindly change things in the kernel without at
least compile testing them.
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging : android: binder.c: Prefer seq_puts to seq_printf
On 02/10/13 11:57, Mathieu Rhéaume wrote:
This patch changes seq_printf for seq_puts in binder.c.
It fixes the warnings emitted by checkpatch.pl.
Signed-off-by: Mathieu Rhéaume math...@codingrhemes.com
---
drivers/staging/android/binder.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/android/binder.c
b/drivers/staging/android/binder.c
index 09edebb..f3d4a1f 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -3272,7 +3272,7 @@ binder_defer_work(struct binder_proc *proc, enum
binder_deferred_state defer)
static void print_binder_transaction(struct seq_file *m, const char *prefix,
struct binder_transaction *t)
{
- seq_printf(m,
+ seq_puts(m,
%s %d: %p from %d:%d to %d:%d code %x flags %x pri %ld r%d,
prefix, t-debug_id, t,
t-from ? t-from-proc-pid : 0,
Umm, this won't compile. seq_puts() is defined as:
int seq_puts(struct seq_file *m, const char *s);
checkpatch.pl does indeed complain about uses of seq_printf() in
drivers/staging/android/binder.c which are passing formatted strings. At
a quick glance it looks like checkpatch.pl is only checking for the
format string on the same line as seq_printf(), so this files ugly
coding style confuses the check. Joe?
Anyway, please don't blindly change things in the kernel without at
least compile testing them.
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] printk: Check real user/group id for %pK
On 30/09/13 10:41, Dan Rosenberg wrote: > On 09/29/2013 07:41 PM, George Spelvin wrote: >>> Right, so the pppd application is actually doing the correct thing. >> And a CAP_SYSLOG setuid binary that *doesn't* DTRT seems like a more >> immediate security hole than leaking kernel addresses. After all >> kptr_restrict is optional precisely because the benefit is marginal. >> >> The interesting question is what credentials make sense for %pK outside >> of a seq_printf(). Does it even make sense in a generic printk? In that >> case, it's the permission of the syslogd that matters rather than the >> process generating the message. >> >>> Will wait and see what others have to say. >> Me, too. Dan in particular. > > Firstly, I wouldn't recommend applying %pK's to printk usage. Sorry, the patch description should say 'vsprintf: ' not 'printk: '. Posting too early in the morning :-). > Removing > all addresses from the kernel syslog compromises its usefulness in > debugging basically anything at all. Additionally, many printk calls are > performed from a context where a capability check would yield > unpredictable (or at least meaningless) results. If you want to restrict > access to the kernel syslog by unprivileged users, that should be done > by enabling CONFIG_DMESG_RESTRICT, which was written for this purpose. Agreed. > With that out of the way, I don't have a strong opinion on how to handle > this case. The proposed patch solves the problem but may break setuid > applications that expect to be able to read /proc/kallsyms contents > based on euid (and implicitly, capabilities) alone. But then again, > these mythical setuid applications are probably broken in some > situations anyway, because what happens if /proc/kallsyms is set to "2" > (unconditionally replace addresses with 0's)? I also can't think of a > better solution. Okay, this was just the simplest solution I could come up with that fixed the issue for me. Is that a tentative acked/reviewed-by? :-). ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] printk: Check real user/group id for %pK
On 30/09/13 09:15, George Spelvin wrote:
> The basic idea is good, but I'm not sure if this is the correct permission
> check to use.
>
> After all, a setuid program might also want to give filtered access to
> a /proc file with some %pK values.
Yeah. I'm not sure if this will break some applications (e.g. perf?)
which are expecting to be able to read %pK values if invoked setuid. The
problem is that allowing that can potentially be used to leak
information too.
>
> The fundamental problem is that %pK is using permissions at the time
> of the read(), while the general Unix rule that setuid programs expect
> is that permission is checked at open() time. pppd is an example; its
> options_fom_file() function (pppd/options.c:391 in the 2.4.5 release)
> does:
>
> euid = geteuid();
> if (check_prot && seteuid(getuid()) == -1) {
> option_error("unable to drop privileges to open %s: %m", filename);
> return 0;
> }
> f = fopen(filename, "r");
> err = errno;
> if (check_prot && seteuid(euid) == -1)
> fatal("unable to regain privileges");
>
Right, so the pppd application is actually doing the correct thing.
>
> Now the whole struct cred and capability system is something I don't
> really understand, but it is clear from a brief look at the code
> that getting the appropriate credential through the seq_file to
> lib/vsprintf.c:pointer() would be tricky.
:-).
>
> But it also seems like the Right Thing to do; other fixes seem like
> ineffective kludges.
Will wait and see what others have to say.
(also apologies for the badly formatted initial post. The mail client on
my other machine is apparently not configured properly).
Thanks,
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] printk: Check real user/group id for %pK
Some setuid binaries will allow reading of files which have read permission by
the real user id. This is problematic with files which use %pK because the
contents of the file is different when opened as root, and displaying the
contents may leak kernel pointer values.
This happens for example with the setuid pppd application on Ubuntu 12.04:
$ head -1 /proc/kallsyms
T startup_32
$ pppd file /proc/kallsyms
pppd: In file /proc/kallsyms: unrecognized option 'c100'
This will only leak the pointer value from the first line, but other setuid
binaries may leak more information.
Fix this by adding a check that in addition to the current process having
CAP_SYSLOG, that effective user and group ids are equal to the real ids. If a
setuid binary reads the contents of a file which uses %pK then the pointer
values will be printed as NULL if the real user is unprivileged.
Signed-off-by: Ryan Mallon
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..b1cd14d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1312,10 +1312,24 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
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
- ptr = NULL;
+
+ /*
+* If kptr_restrict is set to 2, then %pK always prints as
+* NULL. If it is set to 1, then only print the real pointer
+* value if the current proccess has CAP_SYSLOG and is running
+* with the same credentials it started with. We don't want
+* badly written setuid binaries being able to read the real
+* pointers on behalf of unprivileged users.
+*/
+ {
+ const struct cred *cred = current_cred();
+
+ if (kptr_restrict == 2 || (kptr_restrict == 1 &&
+(!has_capability_noaudit(current, CAP_SYSLOG) ||
+ !uid_eq(cred->euid, cred->uid) ||
+ !gid_eq(cred->egid, cred->gid
+ ptr = NULL;
+ }
break;
case 'N':
switch (fmt[1]) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] printk: Check real user/group id for %pK
Some setuid binaries will allow reading of files which have read permission by
the real user id. This is problematic with files which use %pK because the
contents of the file is different when opened as root, and displaying the
contents may leak kernel pointer values.
This happens for example with the setuid pppd application on Ubuntu 12.04:
$ head -1 /proc/kallsyms
T startup_32
$ pppd file /proc/kallsyms
pppd: In file /proc/kallsyms: unrecognized option 'c100'
This will only leak the pointer value from the first line, but other setuid
binaries may leak more information.
Fix this by adding a check that in addition to the current process having
CAP_SYSLOG, that effective user and group ids are equal to the real ids. If a
setuid binary reads the contents of a file which uses %pK then the pointer
values will be printed as NULL if the real user is unprivileged.
Signed-off-by: Ryan Mallon rmal...@gmail.com
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..b1cd14d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1312,10 +1312,24 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
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
- ptr = NULL;
+
+ /*
+* If kptr_restrict is set to 2, then %pK always prints as
+* NULL. If it is set to 1, then only print the real pointer
+* value if the current proccess has CAP_SYSLOG and is running
+* with the same credentials it started with. We don't want
+* badly written setuid binaries being able to read the real
+* pointers on behalf of unprivileged users.
+*/
+ {
+ const struct cred *cred = current_cred();
+
+ if (kptr_restrict == 2 || (kptr_restrict == 1
+(!has_capability_noaudit(current, CAP_SYSLOG) ||
+ !uid_eq(cred-euid, cred-uid) ||
+ !gid_eq(cred-egid, cred-gid
+ ptr = NULL;
+ }
break;
case 'N':
switch (fmt[1]) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] printk: Check real user/group id for %pK
On 30/09/13 09:15, George Spelvin wrote:
The basic idea is good, but I'm not sure if this is the correct permission
check to use.
After all, a setuid program might also want to give filtered access to
a /proc file with some %pK values.
Yeah. I'm not sure if this will break some applications (e.g. perf?)
which are expecting to be able to read %pK values if invoked setuid. The
problem is that allowing that can potentially be used to leak
information too.
The fundamental problem is that %pK is using permissions at the time
of the read(), while the general Unix rule that setuid programs expect
is that permission is checked at open() time. pppd is an example; its
options_fom_file() function (pppd/options.c:391 in the 2.4.5 release)
does:
euid = geteuid();
if (check_prot seteuid(getuid()) == -1) {
option_error(unable to drop privileges to open %s: %m, filename);
return 0;
}
f = fopen(filename, r);
err = errno;
if (check_prot seteuid(euid) == -1)
fatal(unable to regain privileges);
Right, so the pppd application is actually doing the correct thing.
Now the whole struct cred and capability system is something I don't
really understand, but it is clear from a brief look at the code
that getting the appropriate credential through the seq_file to
lib/vsprintf.c:pointer() would be tricky.
:-).
But it also seems like the Right Thing to do; other fixes seem like
ineffective kludges.
Will wait and see what others have to say.
(also apologies for the badly formatted initial post. The mail client on
my other machine is apparently not configured properly).
Thanks,
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] printk: Check real user/group id for %pK
On 30/09/13 10:41, Dan Rosenberg wrote: On 09/29/2013 07:41 PM, George Spelvin wrote: Right, so the pppd application is actually doing the correct thing. And a CAP_SYSLOG setuid binary that *doesn't* DTRT seems like a more immediate security hole than leaking kernel addresses. After all kptr_restrict is optional precisely because the benefit is marginal. The interesting question is what credentials make sense for %pK outside of a seq_printf(). Does it even make sense in a generic printk? In that case, it's the permission of the syslogd that matters rather than the process generating the message. Will wait and see what others have to say. Me, too. Dan in particular. Firstly, I wouldn't recommend applying %pK's to printk usage. Sorry, the patch description should say 'vsprintf: ' not 'printk: '. Posting too early in the morning :-). Removing all addresses from the kernel syslog compromises its usefulness in debugging basically anything at all. Additionally, many printk calls are performed from a context where a capability check would yield unpredictable (or at least meaningless) results. If you want to restrict access to the kernel syslog by unprivileged users, that should be done by enabling CONFIG_DMESG_RESTRICT, which was written for this purpose. Agreed. With that out of the way, I don't have a strong opinion on how to handle this case. The proposed patch solves the problem but may break setuid applications that expect to be able to read /proc/kallsyms contents based on euid (and implicitly, capabilities) alone. But then again, these mythical setuid applications are probably broken in some situations anyway, because what happens if /proc/kallsyms is set to 2 (unconditionally replace addresses with 0's)? I also can't think of a better solution. Okay, this was just the simplest solution I could come up with that fixed the issue for me. Is that a tentative acked/reviewed-by? :-). ~Ryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/12] ping.h: Remove extern from function prototypes
On 23/09/13 12:16, Joe Perches wrote:
> On Mon, 2013-09-23 at 11:59 +1000, Ryan Mallon wrote:
>> This seems like a lot of code churn for very little benefit. At a quick
>> glance:
>>
>> git grep extern include/ | wc -l
>> 11427
>>
>> Not all of those will need to be removed, but that is still a huge
>> number to change, and doesn't include extern usage in C files or local
>> headers. You are probably never going to remove all the instances, so
>> what is the point of just randomly doing a handful?
>
> Rather more than a handful.
>
> The ratio of function prototypes without extern to
> function prototypes with extern is currently ~2.5:1
>
> So:
>
> Standardization without extern
> Line count reduction (~10%)
> Miscellaneous neatening at the same time
> Removal of all unnecessary externs from include/net
>
> There are ~8500 instances in include/
> There are ~1500 instances in include/net/
>
> After this series, 0 in include/net/
>
> Start somewhere, go from there...
>
> $ git grep -E "^\s*\bextern(\s+\w+){1,4}\s*\(\s*[^\*]" include/ | wc -l
> 8395
> $ git grep -E "^\s*\bextern(\s+\w+){1,4}\s*\(\s*[^\*]" include/net/ | wc -l
> 1471
Right, and:
$ git grep -E "^\s*\bextern(\s+\w+){1,4}\s*\(\s*[^\*]" | wc -l
29104
Since there are lots of local/arch headers, and there are uses of extern
function prototypes in C files.
I don't see the real benefit though. Its like trying to "clean-up" the
difference between "unsigned x" and "unsigned int x", or any number of
other minor style differences. Either version, with or without the
extern, is correct, valid C code. Plus you will get people adding new
instances of extern because they don't know any better. A checkpatch
rule might help, but we all know how often people run that...
~Ryan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/12] ping.h: Remove extern from function prototypes
On 23/09/13 03:32, Joe Perches wrote: > There are a mix of function prototypes with and without extern > in the kernel sources. Standardize on not using extern for > function prototypes. > > Function prototypes don't need to be written with extern. > extern is assumed by the compiler. Its use is as unnecessary as > using auto to declare automatic/local variables in a block. > > Signed-off-by: Joe Perches This seems like a lot of code churn for very little benefit. At a quick glance: git grep extern include/ | wc -l 11427 Not all of those will need to be removed, but that is still a huge number to change, and doesn't include extern usage in C files or local headers. You are probably never going to remove all the instances, so what is the point of just randomly doing a handful? ~Ryan > --- > include/net/ping.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/net/ping.h b/include/net/ping.h > index 5db0224..3f67704 100644 > --- a/include/net/ping.h > +++ b/include/net/ping.h > @@ -103,8 +103,8 @@ void ping_seq_stop(struct seq_file *seq, void *v); > int ping_proc_register(struct net *net, struct ping_seq_afinfo *afinfo); > void ping_proc_unregister(struct net *net, struct ping_seq_afinfo *afinfo); > > -extern int __init ping_proc_init(void); > -extern void ping_proc_exit(void); > +int __init ping_proc_init(void); > +void ping_proc_exit(void); > #endif > > void __init ping_init(void); > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/12] ping.h: Remove extern from function prototypes
On 23/09/13 03:32, Joe Perches wrote: There are a mix of function prototypes with and without extern in the kernel sources. Standardize on not using extern for function prototypes. Function prototypes don't need to be written with extern. extern is assumed by the compiler. Its use is as unnecessary as using auto to declare automatic/local variables in a block. Signed-off-by: Joe Perches j...@perches.com This seems like a lot of code churn for very little benefit. At a quick glance: git grep extern include/ | wc -l 11427 Not all of those will need to be removed, but that is still a huge number to change, and doesn't include extern usage in C files or local headers. You are probably never going to remove all the instances, so what is the point of just randomly doing a handful? ~Ryan --- include/net/ping.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/net/ping.h b/include/net/ping.h index 5db0224..3f67704 100644 --- a/include/net/ping.h +++ b/include/net/ping.h @@ -103,8 +103,8 @@ void ping_seq_stop(struct seq_file *seq, void *v); int ping_proc_register(struct net *net, struct ping_seq_afinfo *afinfo); void ping_proc_unregister(struct net *net, struct ping_seq_afinfo *afinfo); -extern int __init ping_proc_init(void); -extern void ping_proc_exit(void); +int __init ping_proc_init(void); +void ping_proc_exit(void); #endif void __init ping_init(void); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 01/12] ping.h: Remove extern from function prototypes
On 23/09/13 12:16, Joe Perches wrote:
On Mon, 2013-09-23 at 11:59 +1000, Ryan Mallon wrote:
This seems like a lot of code churn for very little benefit. At a quick
glance:
git grep extern include/ | wc -l
11427
Not all of those will need to be removed, but that is still a huge
number to change, and doesn't include extern usage in C files or local
headers. You are probably never going to remove all the instances, so
what is the point of just randomly doing a handful?
Rather more than a handful.
The ratio of function prototypes without extern to
function prototypes with extern is currently ~2.5:1
So:
Standardization without extern
Line count reduction (~10%)
Miscellaneous neatening at the same time
Removal of all unnecessary externs from include/net
There are ~8500 instances in include/
There are ~1500 instances in include/net/
After this series, 0 in include/net/
Start somewhere, go from there...
$ git grep -E ^\s*\bextern(\s+\w+){1,4}\s*\(\s*[^\*] include/ | wc -l
8395
$ git grep -E ^\s*\bextern(\s+\w+){1,4}\s*\(\s*[^\*] include/net/ | wc -l
1471
Right, and:
$ git grep -E ^\s*\bextern(\s+\w+){1,4}\s*\(\s*[^\*] | wc -l
29104
Since there are lots of local/arch headers, and there are uses of extern
function prototypes in C files.
I don't see the real benefit though. Its like trying to clean-up the
difference between unsigned x and unsigned int x, or any number of
other minor style differences. Either version, with or without the
extern, is correct, valid C code. Plus you will get people adding new
instances of extern because they don't know any better. A checkpatch
rule might help, but we all know how often people run that...
~Ryan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
