* Patch "fs/proc: Stop trying to report thread stacks" has been added to the 3.18-stable tree
From: gregkh @ 2018-04-06 7:51 UTC (permalink / raw)
To: luto, akpm, bp, brgerst, gregkh, hannes, jann, keescook,
linux-api, mingo, peterz, tglx, torvalds, tycho.andersen, viro
Cc: stable, stable-commits
This is a note to let you know that I've just added the patch titled
fs/proc: Stop trying to report thread stacks
to the 3.18-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
fs-proc-stop-trying-to-report-thread-stacks.patch
and it can be found in the queue-3.18 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.
>From b18cb64ead400c01bf1580eeba330ace51f8087d Mon Sep 17 00:00:00 2001
From: Andy Lutomirski <luto@kernel.org>
Date: Fri, 30 Sep 2016 10:58:57 -0700
Subject: fs/proc: Stop trying to report thread stacks
From: Andy Lutomirski <luto@kernel.org>
commit b18cb64ead400c01bf1580eeba330ace51f8087d upstream.
This reverts more of:
b76437579d13 ("procfs: mark thread stack correctly in proc/<pid>/maps")
... which was partially reverted by:
65376df58217 ("proc: revert /proc/<pid>/maps [stack:TID] annotation")
Originally, /proc/PID/task/TID/maps was the same as /proc/TID/maps.
In current kernels, /proc/PID/maps (or /proc/TID/maps even for
threads) shows "[stack]" for VMAs in the mm's stack address range.
In contrast, /proc/PID/task/TID/maps uses KSTK_ESP to guess the
target thread's stack's VMA. This is racy, probably returns garbage
and, on arches with CONFIG_TASK_INFO_IN_THREAD=y, is also crash-prone:
KSTK_ESP is not safe to use on tasks that aren't known to be running
ordinary process-context kernel code.
This patch removes the difference and just shows "[stack]" for VMAs
in the mm's stack range. This is IMO much more sensible -- the
actual "stack" address really is treated specially by the VM code,
and the current thread stack isn't even well-defined for programs
that frequently switch stacks on their own.
Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linux API <linux-api@vger.kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tycho Andersen <tycho.andersen@canonical.com>
Link: http://lkml.kernel.org/r/3e678474ec14e0a0ec34c611016753eea2e1b8ba.1475257877.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Documentation/filesystems/proc.txt | 26 --------------------------
fs/proc/task_mmu.c | 29 ++++++++++-------------------
fs/proc/task_nommu.c | 26 +++++++++-----------------
3 files changed, 19 insertions(+), 62 deletions(-)
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -371,32 +371,6 @@ is not associated with a file:
or if empty, the mapping is anonymous.
-The /proc/PID/task/TID/maps is a view of the virtual memory from the viewpoint
-of the individual tasks of a process. In this file you will see a mapping marked
-as [stack] if that task sees it as a stack. Hence, for the example above, the
-task-level map, i.e. /proc/PID/task/TID/maps for thread 1001 will look like this:
-
-08048000-08049000 r-xp 00000000 03:00 8312 /opt/test
-08049000-0804a000 rw-p 00001000 03:00 8312 /opt/test
-0804a000-0806b000 rw-p 00000000 00:00 0 [heap]
-a7cb1000-a7cb2000 ---p 00000000 00:00 0
-a7cb2000-a7eb2000 rw-p 00000000 00:00 0
-a7eb2000-a7eb3000 ---p 00000000 00:00 0
-a7eb3000-a7ed5000 rw-p 00000000 00:00 0 [stack]
-a7ed5000-a8008000 r-xp 00000000 03:00 4222 /lib/libc.so.6
-a8008000-a800a000 r--p 00133000 03:00 4222 /lib/libc.so.6
-a800a000-a800b000 rw-p 00135000 03:00 4222 /lib/libc.so.6
-a800b000-a800e000 rw-p 00000000 00:00 0
-a800e000-a8022000 r-xp 00000000 03:00 14462 /lib/libpthread.so.0
-a8022000-a8023000 r--p 00013000 03:00 14462 /lib/libpthread.so.0
-a8023000-a8024000 rw-p 00014000 03:00 14462 /lib/libpthread.so.0
-a8024000-a8027000 rw-p 00000000 00:00 0
-a8027000-a8043000 r-xp 00000000 03:00 8317 /lib/ld-linux.so.2
-a8043000-a8044000 r--p 0001b000 03:00 8317 /lib/ld-linux.so.2
-a8044000-a8045000 rw-p 0001c000 03:00 8317 /lib/ld-linux.so.2
-aff35000-aff4a000 rw-p 00000000 00:00 0
-ffffe000-fffff000 r-xp 00000000 00:00 0 [vdso]
-
The /proc/PID/smaps is an extension based on maps, showing the memory
consumption for each of the process's mappings. For each of mappings there
is a series of lines such as the following:
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -248,24 +248,15 @@ static int do_maps_open(struct inode *in
* /proc/PID/maps that is the stack of the main task.
*/
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
- int stack = 0;
-
- if (is_pid) {
- stack = vma->vm_start <= vma->vm_mm->start_stack &&
- vma->vm_end >= vma->vm_mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= vma->vm_mm->start_stack &&
+ vma->vm_end >= vma->vm_mm->start_stack;
}
static void
@@ -332,7 +323,7 @@ show_map_vma(struct seq_file *m, struct
goto done;
}
- if (is_stack(priv, vma, is_pid))
+ if (is_stack(priv, vma))
name = "[stack]";
}
@@ -1501,7 +1492,7 @@ static int show_numa_map(struct seq_file
seq_path(m, &file->f_path, "\n\t= ");
} else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
seq_puts(m, " heap");
- } else if (is_stack(proc_priv, vma, is_pid)) {
+ } else if (is_stack(proc_priv, vma)) {
seq_puts(m, " stack");
}
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -124,25 +124,17 @@ unsigned long task_statm(struct mm_struc
}
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
struct mm_struct *mm = vma->vm_mm;
- int stack = 0;
- if (is_pid) {
- stack = vma->vm_start <= mm->start_stack &&
- vma->vm_end >= mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= mm->start_stack &&
+ vma->vm_end >= mm->start_stack;
}
/*
@@ -184,7 +176,7 @@ static int nommu_vma_show(struct seq_fil
if (file) {
seq_pad(m, ' ');
seq_path(m, &file->f_path, "");
- } else if (mm && is_stack(priv, vma, is_pid)) {
+ } else if (mm && is_stack(priv, vma)) {
seq_pad(m, ' ');
seq_printf(m, "[stack]");
}
Patches currently in stable-queue which might be from luto@kernel.org are
queue-3.18/perf-hwbp-simplify-the-perf-hwbp-code-fix-documentation.patch
queue-3.18/fs-proc-stop-trying-to-report-thread-stacks.patch
queue-3.18/kvm-x86-fix-icebp-instruction-handling.patch
^ permalink raw reply
* Re: [PATCH 3/6] aio: refactor read/write iocb setup
From: Christoph Hellwig @ 2018-04-06 7:10 UTC (permalink / raw)
To: Al Viro
Cc: Christoph Hellwig, Avi Kivity, linux-aio, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <20180406032146.GV30522@ZenIV.linux.org.uk>
On Fri, Apr 06, 2018 at 04:21:46AM +0100, Al Viro wrote:
> On Wed, Mar 28, 2018 at 09:26:36AM +0200, Christoph Hellwig wrote:
> > + struct inode *inode = file_inode(file);
> > +
> > req->ki_flags |= IOCB_WRITE;
> > file_start_write(file);
> > - ret = aio_ret(req, call_write_iter(file, req, &iter));
> > + ret = aio_rw_ret(req, call_write_iter(file, req, &iter));
> > /*
> > - * We release freeze protection in aio_complete(). Fool lockdep
> > - * by telling it the lock got released so that it doesn't
> > - * complain about held lock when we return to userspace.
> > + * We release freeze protection in aio_complete_rw(). Fool
> > + * lockdep by telling it the lock got released so that it
> > + * doesn't complain about held lock when we return to userspace.
> > */
> > - if (S_ISREG(file_inode(file)->i_mode))
> > - __sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> > + if (S_ISREG(inode->i_mode))
>
> ... and that's another use-after-free, since we might've already done fput() of
> that sucker by that point.
Indeed. Not in any way new in this patch, this is an existing issue
dating way back that needs to be fixed, which will be rather annoying
without taking an extra reference to the inode or at least sb.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: io_pgetevents & aio fsync V2
From: Christoph Hellwig @ 2018-04-06 6:27 UTC (permalink / raw)
To: Al Viro
Cc: Christoph Hellwig, Avi Kivity, linux-aio, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <20180406031630.GU30522@ZenIV.linux.org.uk>
On Fri, Apr 06, 2018 at 04:16:30AM +0100, Al Viro wrote:
> BTW, this is only tangentially related, but... does *anything* call
> io_submit() for huge amounts of iocb? Check in do_io_submit() is
> insane - "no more than MAX_LONG total of _pointers_". Compat variant
> goes for "no more than a page worth of pointers" and there's
> a hard limit in ioctx_alloc() - we can't ever get more than
> 8M slots in ring buffer...
Logical upper bound for io_submit is nr_events passed to io_setup(),
which is bound by aio_max_nr. Except that we never actually check
against nr_events (or max_reqs as it is known in kernel) in io_submit.
Sigh..
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/32] fs: introduce new ->get_poll_head and ->poll_mask methods
From: Al Viro @ 2018-04-06 4:58 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180330150809.28094-6-hch@lst.de>
On Fri, Mar 30, 2018 at 05:07:42PM +0200, Christoph Hellwig wrote:
> + get_poll_head: Returns the struct wait_queue_head that poll, select,
> + epoll or aio poll should wait on in case this instance only has single
> + waitqueue. Can return NULL to indicate polling is not supported,
> + or a POLL* value using the POLL_TO_PTR helper in case a grave error
> + occured and ->poll_mask shall not be called.
> + if (IS_ERR(head))
> + return PTR_TO_POLL(head);
> + * ->get_poll_head can return a __poll_t in the PTR_ERR, use these macros
> + * to return the value and recover it. It takes care of the negation as
> + * well as off the annotations.
> + */
> +#define POLL_TO_PTR(mask) (ERR_PTR(-(__force int)(mask)))
Uh-oh...
static inline bool __must_check IS_ERR(__force const void *ptr)
{
return IS_ERR_VALUE((unsigned long)ptr);
}
#define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)
#define MAX_ERRNO 4095
IOW, your trick relies upon arguments of PTR_TO_POLL being no greater
than 4095. Now, consider
#define EPOLLRDHUP (__force __poll_t)0x00002000
which is to say, 8192...
So anything that tries e.g. POLL_TO_PTR(EPOLLRDHUP | EPOLLERR) will be in
for a quiet unpleasant surprise...
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Peter Dolding @ 2018-04-06 4:42 UTC (permalink / raw)
To: Peter Jones
Cc: Andy Lutomirski, Matthew Garrett, David Howells, Ard Biesheuvel,
James Morris, Alan Cox, Linus Torvalds, Greg Kroah-Hartman,
Linux Kernel Mailing List, Justin Forbes, linux-man, joeyli,
LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <20180404184255.exdrtpqnxlqme7tl@redhat.com>
>
> There's no inherent difference, in terms of the trust chain, between
> compromising it to use the machine as a toaster or to run a botnet - the
> trust chain is compromised either way. But you're much more likely to
> notice if your desktop starts producing bread products than if it hides
> some malware and keeps on booting, and the second one is much more
> That is to say, as a result of the way malware has been written, our way
> of thinking about it is often that it's a way to build a boot loader for
> a malicious kernel, so that's how we wind up talking about it. Are we
> concerned with malware stealing your data? Yes, but Secure Boot is only
> indirectly about that. It's primarily about denying the malware easy
> mechanisms to build a persistence mechanism. The uid-0 != ring-0 aspect
> is useful independent of Secure Boot, but Secure Boot without it falls
> way short of accomplishing its goal.
>
> --
I am sorry the issue here this is really expanding Secure Boot to
breaking point.
Yes a person wants a secure system having the boot parts verified by
some means and using a lockdown is advantage.
Problem comes in with the idea that UEFI Secure Boot and lockdown are linked.
If I am running windows and linux on the same machine Secure Boot need
to be on so windows run happy.
Remember its my machine. If I wish to compromise security on my
machine because it make sense I should be allowed to,
A proper lockdown would prevent you from messing with ACPI tables it a
very creative hack have kernel load a DSDT and have it from ring zero
turn bits in the kernel off.
The reality here is we need to be able to operate without lockdown due
to how badly broken some hardware it to configure system.
Yes the need to include option to push button to disable secure boot
is required due to how badly broken this stuff is. Of course this
does not address the issue that if I am working on a system from
remote or embedded where I don't have the push button to turn off as
a option this is still a problem.
Effective lockdown has to protect linux kernel boot parameters,
initramfs and other bits from being modified as well. This lead us
to problem with the broken hardware in a machine we cannot turn secure
boot off we still need to perform all these alterations.
We do not live in a world of perfect computer hardware so at this
stage proper unattackable secureboot cannot be done.
We would be better off putting effort into improve means with UEFI of
adding own KEK. This is so that only boot loaders and kernels from
the vendors user has approved in fact to work. There could also be a
configuration KEK that gets disabled after all the required operating
systems are installed. So Microsoft non OS KEK makes sense to be
the configuration rule breaking KEK but the current deployments of
UEFI don't have a off switch option on it.
One KEK for everyone who is not Microsoft to boot with is highly insecure.
UEFI secureboot falls way short in the validation department currently
because too much is validated under one KEK key.
UEFI also fall short due to failing to provide a system to protect
boot parameters that can alter OS behaviour and make a secure kernel
insecure this include kernels with this lockdown patches,
Really you need to compare UEFI secureboot vs boot loader and /boot on
a read only media. Every where you can change something in the UEFI
secureboot without is being signed that you cannot in the read only
media of the boot loader and /boot is a defect in the UEFI secureboot
design and implementation.
If boot parameters were properly secured there would be no need for
lockdown query if UEFI was in secureboot mode or not.
Also lockdown being on and kernel and boot loader not running secured
still would provide extra item attacker has to get past.
So fairly much remove the EFI interrogation patches and work with UEFI
to fix it properly. Hacking around these UEFI defects means we will
end up being stuck with them and the system still not being properly
secured.
Peter Dolding
^ permalink raw reply
* Re: [PATCH 3/6] aio: refactor read/write iocb setup
From: Al Viro @ 2018-04-06 3:21 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180328072639.16885-4-hch@lst.de>
On Wed, Mar 28, 2018 at 09:26:36AM +0200, Christoph Hellwig wrote:
> + struct inode *inode = file_inode(file);
> +
> req->ki_flags |= IOCB_WRITE;
> file_start_write(file);
> - ret = aio_ret(req, call_write_iter(file, req, &iter));
> + ret = aio_rw_ret(req, call_write_iter(file, req, &iter));
> /*
> - * We release freeze protection in aio_complete(). Fool lockdep
> - * by telling it the lock got released so that it doesn't
> - * complain about held lock when we return to userspace.
> + * We release freeze protection in aio_complete_rw(). Fool
> + * lockdep by telling it the lock got released so that it
> + * doesn't complain about held lock when we return to userspace.
> */
> - if (S_ISREG(file_inode(file)->i_mode))
> - __sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> + if (S_ISREG(inode->i_mode))
... and that's another use-after-free, since we might've already done fput() of
that sucker by that point.
> + __sb_writers_release(inode->i_sb, SB_FREEZE_WRITE);
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: io_pgetevents & aio fsync V2
From: Al Viro @ 2018-04-06 3:16 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180328072639.16885-1-hch@lst.de>
On Wed, Mar 28, 2018 at 09:26:33AM +0200, Christoph Hellwig wrote:
> Hi all,
>
> this patch adds workqueue based fsync offload. Version of this
> patch have been floating around for a couple years, but we now
> have a user with seastar used by ScyllaDB (who sponsored this
> work) that really wants this in addition to the aio poll support.
> More details are in the patch itself.
>
> Because the iocb types have been defined sine day one (and probably
> were supported by RHEL3) libaio already supports these calls as-is.
>
> This also pulls in the aio cleanups and io_pgetevents support previously
> submitted and review as part of the aio poll series. The aio poll
> series will be resubmitted on top of this series
BTW, this is only tangentially related, but... does *anything* call
io_submit() for huge amounts of iocb? Check in do_io_submit() is
insane - "no more than MAX_LONG total of _pointers_". Compat variant
goes for "no more than a page worth of pointers" and there's
a hard limit in ioctx_alloc() - we can't ever get more than
8M slots in ring buffer...
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 5/6] aio: implement IOCB_CMD_FSYNC and IOCB_CMD_FDSYNC
From: Al Viro @ 2018-04-06 2:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180328072639.16885-6-hch@lst.de>
On Wed, Mar 28, 2018 at 09:26:38AM +0200, Christoph Hellwig wrote:
> +static int aio_fsync(struct fsync_iocb *req, struct iocb *iocb, bool datasync)
> +{
> + int ret;
> +
> + if (iocb->aio_buf)
> + return -EINVAL;
> + if (iocb->aio_offset || iocb->aio_nbytes || iocb->aio_rw_flags)
> + return -EINVAL;
> +
> + req->file = fget(iocb->aio_fildes);
> + if (unlikely(!req->file))
> + return -EBADF;
> +
> + ret = -EINVAL;
> + if (!req->file->f_op->fsync)
> + goto out_fput;
> +
> + req->datasync = datasync;
> + INIT_WORK(&req->work, aio_fsync_work);
> + schedule_work(&req->work);
> + return -EIOCBQUEUED;
> +out_fput:
> + if (unlikely(ret && ret != -EIOCBQUEUED))
Really? The only way to get there is if we got NULL ->f_op->fsync.
So this "if (unlikely(...))" is actually if (true) and I'm not
sure we want that separated anyway - simply
if (unlikely(!req->file->f_op->fsync)) {
fput(req->file);
return -EINVAL;
}
> + fput(req->file);
> + return ret;
> +}
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 08/32] aio: replace kiocb_set_cancel_fn with a cancel_kiocb file operation
From: Al Viro @ 2018-04-06 2:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180330150809.28094-9-hch@lst.de>
On Fri, Mar 30, 2018 at 05:07:45PM +0200, Christoph Hellwig wrote:
> The current kiocb_set_cancel_fn implementation assumes the kiocb is
> embedded into an aio_kiocb, which is fundamentally unsafe as it might
> have been submitted by non-aio callers. Instead add a cancel_kiocb
> file operation that replaced the ki_cancel function pointer set by
> kiocb_set_cancel_fn, and only adds iocbs to the active list when
> the read/write_iter methods return -EIOCBQUEUED and the file has
> a cancel_kiocb method.
> @@ -1440,6 +1423,16 @@ static inline ssize_t aio_rw_ret(struct kiocb *req, ssize_t ret)
> {
> switch (ret) {
> case -EIOCBQUEUED:
> + if (req->ki_filp->f_op->cancel_kiocb) {
... and by that point req might've been already freed by IO completion on
another CPU.
> + struct aio_kiocb *iocb =
> + container_of(req, struct aio_kiocb, rw);
> + struct kioctx *ctx = iocb->ki_ctx;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ctx->ctx_lock, flags);
> + list_add_tail(&iocb->ki_list, &ctx->active_reqs);
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Andy Lutomirski @ 2018-04-05 18:47 UTC (permalink / raw)
To: Peter Jones
Cc: Andy Lutomirski, Matthew Garrett, David Howells, Ard Biesheuvel,
James Morris, Alan Cox, Linus Torvalds, Greg Kroah-Hartman,
Linux Kernel Mailing List, Justin Forbes, linux-man, joeyli,
LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <20180404184255.exdrtpqnxlqme7tl@redhat.com>
On Wed, Apr 4, 2018 at 11:42 AM, Peter Jones <pjones@redhat.com> wrote:
> On Tue, Apr 03, 2018 at 02:51:23PM -0700, Andy Lutomirski wrote:
>> On Tue, Apr 3, 2018 at 12:29 PM, Matthew Garrett <mjg59@google.com> wrote:
>> Can someone please explain why the UEFI crowd cares so much about "as
>> a bootloader"? Once I'm able to install an OS (Linux kernel +
>> bootloader, Windows embedded doodad, OpenBSD, whatever) on your
>> machine, I can use your peripherals, read your data, write your data,
>> see your keystrokes, use your network connection, re-flash your BIOS
>> (at least as well as any OS can), run VMs, and generally own your
>> system. Somehow you all seem fine with all of this, except that the
>> fact that I can chainload something else gives UEFI people the
>> willies.
>>
>> Can someone explain why?
>
> There's no inherent difference, in terms of the trust chain, between
> compromising it to use the machine as a toaster or to run a botnet - the
> trust chain is compromised either way. But you're much more likely to
> notice if your desktop starts producing bread products than if it hides
> some malware and keeps on booting, and the second one is much more
> attractive to attackers anyway.
>
> The reason we talk about it as a bootloader is because of the model
> employed by malware. I'm sure you know that one kind of malware that
> exists in the wild, a so-called "boot kit", operates by modifying a
> kernel during load (or on disk before loading) so that it has some
> malicious payload, like exfiltrating user data or allowing a way to
> install software that the kernel hides or *whatever*, and incorporating
> some way to achieve relative persistence on the system - for example
> hiding the real boot settings and loading a kernel with a different than
> normal initramfs that loads an exploit before continuing with a normal
> looking boot.
This is a fair point, but I wonder how much it matters in practice.
If I'm writing a bootkit, I can think of at least four ways to do it.
1. The easy way. Write a malicious bootloader that modifies the
kernel image to insert malicious code. Stock secure boot makes this
awkward because you need a signed bootloader. It's worth noting that
a non-locked-down signed Linux kernel is actually a rather awkward way
to do this because it will add several seconds to the boot and may
show a splash screen unless you're rather careful.
2. The CPL3 way. Write a malicious initramfs that inserts the
malicious code in PID 1 instead. This might be easier to get working
across a variety of Linux kernels, but it's more awkward to hide well
from userspace. Conventional secure boot (with the stock MS keys)
doesn't help at all.
3. The nasty way. Find a known exploitable kernel or bootloader, and
use it to do your evil deeds. This is very, very hard to protect
against with normal secure boot.
4. The VM-kit way. Use a signed, locked down, perfectly secure kernel
and run your pwned system as a VM guest. Secure boot doesn't help one
whit.
*All* of these variants are avoided by a real, working verified boot
approach that chains all the way down to the running system image, and
*that* solution doesn't need cpl0 and cpl3 to be separated.
So I find myself wondering whether the bootkit argument is actually
very compelling.
^ permalink raw reply
* Re: [PATCH v4 16/24] fpga: dfl: add fpga manager platform driver for FME
From: Alan Tull @ 2018-04-05 18:45 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <20180322060722.GA8707@hao-dev>
On Thu, Mar 22, 2018 at 1:07 AM, Wu Hao <hao.wu@intel.com> wrote:
> On Wed, Mar 21, 2018 at 09:55:52AM -0700, Moritz Fischer wrote:
>> On Wed, Mar 21, 2018 at 10:50:01AM +0800, Wu Hao wrote:
>> > On Tue, Mar 20, 2018 at 03:32:34PM -0500, Alan Tull wrote:
>> > > On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
>> > >
>> > > Hi Hao,
>> > >
>> > > Elsewhere we discussed moving #defines used only in this driver either
>> > > to this .c file or to a similarly named .h file. A couple minor
>> > > things below.
>> >
>> > Hi Alan,
>> >
>> > Yes, I will move those #defines into a similarly named .h file.
>> >
>> > >
>> > > > This patch adds fpga manager driver for FPGA Management Engine (FME). It
>> > > > implements fpga_manager_ops for FPGA Partial Reconfiguration function.
>> > > >
>> > > > Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
>> > > > Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
>> > > > Signed-off-by: Shiva Rao <shiva.rao@intel.com>
>> > > > Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
>> > > > Signed-off-by: Kang Luwei <luwei.kang@intel.com>
>> > > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> > > > Signed-off-by: Wu Hao <hao.wu@intel.com>
>> > > > ---
>> > > > v3: rename driver to dfl-fpga-fme-mgr
>> > > > implemented status callback for fpga manager
>> > > > rebased due to fpga api changes
>> > > > v4: rename to dfl-fme-mgr, and fix SPDX license issue
>> > > > add pr_credit comments and improve dev_err message
>> > > > remove interface_id sysfs interface
>> > > > include dfl-fme-pr.h instead of dfl.h
>> > > > ---
>> > > > drivers/fpga/Kconfig | 6 +
>> > > > drivers/fpga/Makefile | 1 +
>> > > > drivers/fpga/dfl-fme-mgr.c | 290 +++++++++++++++++++++++++++++++++++++++++++++
>> > > > 3 files changed, 297 insertions(+)
>> > > > create mode 100644 drivers/fpga/dfl-fme-mgr.c
>> > > >
>> > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> > > > index 103d5e2..89f76e8 100644
>> > > > --- a/drivers/fpga/Kconfig
>> > > > +++ b/drivers/fpga/Kconfig
>> > > > @@ -150,6 +150,12 @@ config FPGA_DFL_FME
>> > > > FPGA platform level management features. There shall be 1 FME
>> > > > per DFL based FPGA device.
>> > > >
>> > > > +config FPGA_DFL_FME_MGR
>> > > > + tristate "FPGA DFL FME Manager Driver"
>> > > > + depends on FPGA_DFL_FME
>> > > > + help
>> > > > + Say Y to enable FPGA Manager driver for FPGA Management Engine.
>> > > > +
>> > > > config FPGA_DFL_PCI
>> > > > tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
>> > > > depends on PCI && FPGA_DFL
>> > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> > > > index 3c44fc9..f82814a 100644
>> > > > --- a/drivers/fpga/Makefile
>> > > > +++ b/drivers/fpga/Makefile
>> > > > @@ -31,6 +31,7 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
>> > > > # FPGA Device Feature List Support
>> > > > obj-$(CONFIG_FPGA_DFL) += dfl.o
>> > > > obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
>> > > > +obj-$(CONFIG_FPGA_DFL_FME_MGR) += dfl-fme-mgr.o
>> > > >
>> > > > dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
>> > > >
>> > > > diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
>> > > > new file mode 100644
>> > > > index 0000000..2f92c29
>> > > > --- /dev/null
>> > > > +++ b/drivers/fpga/dfl-fme-mgr.c
>> > > > @@ -0,0 +1,290 @@
>> > > > +// SPDX-License-Identifier: GPL-2.0
>> > > > +/*
>> > > > + * FPGA Manager Driver for FPGA Management Engine (FME)
>> > > > + *
>> > > > + * Copyright (C) 2017 Intel Corporation, Inc.
>> > > > + *
>> > > > + * Authors:
>> > > > + * Kang Luwei <luwei.kang@intel.com>
>> > > > + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> > > > + * Wu Hao <hao.wu@intel.com>
>> > > > + * Joseph Grecco <joe.grecco@intel.com>
>> > > > + * Enno Luebbers <enno.luebbers@intel.com>
>> > > > + * Tim Whisonant <tim.whisonant@intel.com>
>> > > > + * Ananda Ravuri <ananda.ravuri@intel.com>
>> > > > + * Christopher Rauer <christopher.rauer@intel.com>
>> > > > + * Henry Mitchel <henry.mitchel@intel.com>
>> > > > + */
>> > > > +
>> > > > +#include <linux/bitfield.h>
>> > > > +#include <linux/module.h>
>> > > > +#include <linux/iopoll.h>
>> > > > +#include <linux/fpga/fpga-mgr.h>
>> > > > +
>> > > > +#include "dfl-fme-pr.h"
>> > > > +
>> > > > +#define PR_WAIT_TIMEOUT 8000000
>> > > > +#define PR_HOST_STATUS_IDLE 0
>> > > > +
>> > > > +struct fme_mgr_priv {
>> > > > + void __iomem *ioaddr;
>> > > > + u64 pr_error;
>> > > > +};
>> > > > +
>> > > > +static u64 pr_error_to_mgr_status(u64 err)
>> > > > +{
>> > > > + u64 status = 0;
>> > > > +
>> > > > + if (err & FME_PR_ERR_OPERATION_ERR)
>> > > > + status |= FPGA_MGR_STATUS_OPERATION_ERR;
>> > > > + if (err & FME_PR_ERR_CRC_ERR)
>> > > > + status |= FPGA_MGR_STATUS_CRC_ERR;
>> > > > + if (err & FME_PR_ERR_INCOMPATIBLE_BS)
>> > > > + status |= FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR;
>> > > > + if (err & FME_PR_ERR_PROTOCOL_ERR)
>> > > > + status |= FPGA_MGR_STATUS_IP_PROTOCOL_ERR;
>> > > > + if (err & FME_PR_ERR_FIFO_OVERFLOW)
>> > > > + status |= FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR;
>> > > > +
>> > > > + return status;
>> > > > +}
>> > > > +
>> > > > +static u64 fme_mgr_pr_error_handle(void __iomem *fme_pr)
>> > > > +{
>> > > > + u64 pr_status, pr_error;
>> > > > +
>> > > > + pr_status = readq(fme_pr + FME_PR_STS);
>> > > > + if (!(pr_status & FME_PR_STS_PR_STS))
>> > > > + return 0;
>> > > > +
>> > > > + pr_error = readq(fme_pr + FME_PR_ERR);
>> > > > + writeq(pr_error, fme_pr + FME_PR_ERR);
>> > > > +
>> > > > + return pr_error;
>> > > > +}
>> > > > +
>> > > > +static int fme_mgr_write_init(struct fpga_manager *mgr,
>> > > > + struct fpga_image_info *info,
>> > > > + const char *buf, size_t count)
>> > > > +{
>> > > > + struct device *dev = &mgr->dev;
>> > > > + struct fme_mgr_priv *priv = mgr->priv;
>> > > > + void __iomem *fme_pr = priv->ioaddr;
>> > > > + u64 pr_ctrl, pr_status;
>> > > > +
>> > > > + if (!(info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>> > > > + dev_err(dev, "only supports partial reconfiguration.\n");
>> > > > + return -EINVAL;
>> > > > + }
>> > > > +
>> > > > + dev_dbg(dev, "resetting PR before initiated PR\n");
>> > > > +
>> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
>> > > > + pr_ctrl |= FME_PR_CTRL_PR_RST;
>> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
>> > > > +
>> > > > + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
>> > > > + pr_ctrl & FME_PR_CTRL_PR_RSTACK, 1,
>> > > > + PR_WAIT_TIMEOUT)) {
>> > > > + dev_err(dev, "PR Reset ACK timeout\n");
>> > > > + return -ETIMEDOUT;
>> > > > + }
>> > > > +
>> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
>> > > > + pr_ctrl &= ~FME_PR_CTRL_PR_RST;
>> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
>> > > > +
>> > > > + dev_dbg(dev,
>> > > > + "waiting for PR resource in HW to be initialized and ready\n");
>> > > > +
>> > > > + if (readq_poll_timeout(fme_pr + FME_PR_STS, pr_status,
>> > > > + (pr_status & FME_PR_STS_PR_STS) ==
>> > > > + FME_PR_STS_PR_STS_IDLE, 1, PR_WAIT_TIMEOUT)) {
>> > > > + dev_err(dev, "PR Status timeout\n");
>> > > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
>> > > > + return -ETIMEDOUT;
>> > > > + }
>> > > > +
>> > > > + dev_dbg(dev, "check and clear previous PR error\n");
>> > > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
>> > > > + if (priv->pr_error)
>> > > > + dev_dbg(dev, "previous PR error detected %llx\n",
>> > > > + (unsigned long long)priv->pr_error);
>> > > > +
>> > > > + dev_dbg(dev, "set PR port ID\n");
>> > > > +
>> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
>> > > > + pr_ctrl &= ~FME_PR_CTRL_PR_RGN_ID;
>> > > > + pr_ctrl |= FIELD_PREP(FME_PR_CTRL_PR_RGN_ID, info->region_id);
>> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
>> > > > +
>> > > > + return 0;
>> > > > +}
>> > > > +
>> > > > +static int fme_mgr_write(struct fpga_manager *mgr,
>> > > > + const char *buf, size_t count)
>> > > > +{
>> > > > + struct device *dev = &mgr->dev;
>> > > > + struct fme_mgr_priv *priv = mgr->priv;
>> > > > + void __iomem *fme_pr = priv->ioaddr;
>> > > > + u64 pr_ctrl, pr_status, pr_data;
>> > > > + int delay = 0, pr_credit, i = 0;
>> > > > +
>> > > > + dev_dbg(dev, "start request\n");
>> > > > +
>> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
>> > > > + pr_ctrl |= FME_PR_CTRL_PR_START;
>> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
>> > > > +
>> > > > + dev_dbg(dev, "pushing data from bitstream to HW\n");
>> > > > +
>> > > > + /*
>> > > > + * driver can push data to PR hardware using PR_DATA register once HW
>> > > > + * has enough pr_credit (> 1), pr_credit reduces one for every 32bit
>> > > > + * pr data write to PR_DATA register. If pr_credit <= 1, driver needs
>> > > > + * to wait for enough pr_credit from hardware by polling.
>> > > > + */
>> > > > + pr_status = readq(fme_pr + FME_PR_STS);
>> > > > + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
>> > > > +
>> > > > + while (count > 0) {
>> > > > + while (pr_credit <= 1) {
>> > > > + if (delay++ > PR_WAIT_TIMEOUT) {
>> > > > + dev_err(dev, "PR_CREDIT timeout\n");
>> > > > + return -ETIMEDOUT;
>> > > > + }
>> > > > + udelay(1);
>> > > > +
>> > > > + pr_status = readq(fme_pr + FME_PR_STS);
>> > > > + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
>> > > > + }
>> > > > +
>> > > > + if (count >= 4) {
>> > > > + pr_data = 0;
>> > > > + pr_data |= FIELD_PREP(FME_PR_DATA_PR_DATA_RAW,
>> > > > + *(((u32 *)buf) + i));
>> > > > + writeq(pr_data, fme_pr + FME_PR_DATA);
>> > > > + count -= 4;
>> > > > + pr_credit--;
>> > > > + i++;
>> > > > + } else {
>> > > > + WARN_ON(1);
>> > > > + return -EINVAL;
>> > > > + }
Where there's an if..else that has a 'return' on one of its branch, it
can become an 'if' without the 'else'. This is strongly preferred in
the kernel. Trying to avoid indention where possible. So this
becomes
if (count < 4) {
WARN_ON(1);
return -EINVAL;
}
pr_data = 0;
so on...
Alan
>> > > > + }
>> > > > +
>> > > > + return 0;
>> > > > +}
>> > > > +
>> > > > +static int fme_mgr_write_complete(struct fpga_manager *mgr,
>> > > > + struct fpga_image_info *info)
>> > > > +{
>> > > > + struct device *dev = &mgr->dev;
>> > > > + struct fme_mgr_priv *priv = mgr->priv;
>> > > > + void __iomem *fme_pr = priv->ioaddr;
>> > > > + u64 pr_ctrl;
>> > > > +
>> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
>> > > > + pr_ctrl |= FME_PR_CTRL_PR_COMPLETE;
>> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
>> > > > +
>> > > > + dev_dbg(dev, "green bitstream push complete\n");
>> > > > + dev_dbg(dev, "waiting for HW to release PR resource\n");
>> > > > +
>> > > > + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
>> > > > + !(pr_ctrl & FME_PR_CTRL_PR_START), 1,
>> > > > + PR_WAIT_TIMEOUT)) {
>> > > > + dev_err(dev, "PR Completion ACK timeout.\n");
>> > > > + return -ETIMEDOUT;
>> > > > + }
>> > > > +
>> > > > + dev_dbg(dev, "PR operation complete, checking status\n");
>> > > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
>> > > > + if (priv->pr_error) {
>> > > > + dev_dbg(dev, "PR error detected %llx\n",
>> > > > + (unsigned long long)priv->pr_error);
>> > > > + return -EIO;
>> > > > + }
>> > > > +
>> > > > + dev_dbg(dev, "PR done successfully\n");
>> > > > +
>> > > > + return 0;
>> > > > +}
>> > > > +
>> > > > +static enum fpga_mgr_states fme_mgr_state(struct fpga_manager *mgr)
>> > > > +{
>> > > > + return FPGA_MGR_STATE_UNKNOWN;
>> > > > +}
>> > > > +
>> > > > +static u64 fme_mgr_status(struct fpga_manager *mgr)
>> > > > +{
>> > > > + struct fme_mgr_priv *priv = mgr->priv;
>> > > > +
>> > > > + return pr_error_to_mgr_status(priv->pr_error);
>> > > > +}
>> > > > +
>> > > > +static const struct fpga_manager_ops fme_mgr_ops = {
>> > > > + .write_init = fme_mgr_write_init,
>> > > > + .write = fme_mgr_write,
>> > > > + .write_complete = fme_mgr_write_complete,
>> > > > + .state = fme_mgr_state,
>> > > > + .status = fme_mgr_status,
>> > > > +};
>> > > > +
>> > > > +static int fme_mgr_probe(struct platform_device *pdev)
>> > > > +{
>> > > > + struct device *dev = &pdev->dev;
>> > > > + struct fme_mgr_priv *priv;
>> > > > + struct fpga_manager *mgr;
>> > > > + struct resource *res;
>> > > > + int ret;
>> > > > +
>> > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> > > > + if (!priv)
>> > > > + return -ENOMEM;
>> > > > +
>> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > > > + priv->ioaddr = devm_ioremap(dev, res->start, resource_size(res));
>> > >
>> > > How about using devm_ioremap_resourc(dev, res) here instead?
>> >
>> > Actually the register region has already been mapped in lower level driver
>> > (e.g pci) so I think we don't have to map the second time here. I plan to
>> > add some code to pass the ioaddr via the platform data, and check if valid
>> > ioaddr from the platform data firstly in this probe function. If no pdata
>> > or no valid ioaddr, then go with devm_ioremap_resource. :)
>>
>> If you end up sharing register spaces between drivers is regmap / syscon
>> maybe a good idea?
>
> Hi Moritz,
>
> Thanks for the comments. I tried the regmap_mmio before but it failed to use
> regmap_read and regmap_write for register access. It requires to use writeq /
> readq for 64bit registers on the hardware, but regmap_read and regmap_write
> only accepts unsigned int value as input parameter. So I only replaced the
> bitfields struct/union with macro.
>
> Thanks
> Hao
>
>> >
>> > >
>> > > > + if (IS_ERR(priv->ioaddr))
>> > > > + return PTR_ERR(priv->ioaddr);
>> > > > +
>> > > > + mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
>> > > > + if (!mgr)
>> > > > + return -ENOMEM;
>> > > > +
>> > > > + mgr->name = "DFL FPGA Manager";
>> > > > + mgr->mops = &fme_mgr_ops;
>> > > > + mgr->priv = priv;
>> > > > + mgr->parent = dev;
>> > > > + platform_set_drvdata(pdev, mgr);
>> > > > +
>> > > > + ret = fpga_mgr_register(mgr);
>> > > > + if (ret)
>> > > > + dev_err(dev, "unable to register FPGA manager\n");
>> > > > +
>> > > > + return ret;
>> > >
>> > > You can probably just do "return fpga_mgr_register(mgr);" here.
>> >
>> > Yes, it looks better, I will fix it. Thanks a lot for the review.
>> >
>> > Hao
>> >
>> > >
>> > > Thanks,
>> > > Alan
>> > >
>> > > > +}
>> > > > +
>> > > > +static int fme_mgr_remove(struct platform_device *pdev)
>> > > > +{
>> > > > + struct fpga_manager *mgr = platform_get_drvdata(pdev);
>> > > > +
>> > > > + fpga_mgr_unregister(mgr);
>> > > > +
>> > > > + return 0;
>> > > > +}
>> > > > +
>> > > > +static struct platform_driver fme_mgr_driver = {
>> > > > + .driver = {
>> > > > + .name = FPGA_DFL_FME_MGR,
>> > > > + },
>> > > > + .probe = fme_mgr_probe,
>> > > > + .remove = fme_mgr_remove,
>> > > > +};
>> > > > +
>> > > > +module_platform_driver(fme_mgr_driver);
>> > > > +
>> > > > +MODULE_DESCRIPTION("FPGA Manager for DFL FPGA Management Engine");
>> > > > +MODULE_AUTHOR("Intel Corporation");
>> > > > +MODULE_LICENSE("GPL v2");
>> > > > +MODULE_ALIAS("platform:dfl-fme-mgr");
>> > > > --
>> > > > 2.7.4
>> > > >
>>
>> Cheers,
>> Moritz
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 10/24] fpga: dfl: add FPGA Management Engine driver basic framework
From: Alan Tull @ 2018-04-05 18:35 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1518513893-4719-11-git-send-email-hao.wu@intel.com>
On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> From: Kang Luwei <luwei.kang@intel.com>
>
> The FPGA Management Engine (FME) provides power, thermal management,
> performance counters, partial reconfiguration and other functions. For each
> function, it is packaged into a private feature linked to the FME feature
> device in the 'Device Feature List'. It's a platform device created by
> DFL framework.
>
> This patch adds the basic framework of FME platform driver. It defines
> sub feature drivers to handle the different sub features, including init,
> uinit and ioctl. It also registers the file operations for the device file.
>
> Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> Signed-off-by: Kang Luwei <luwei.kang@intel.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> ---
> v3: rename driver from intel-fpga-fme to dfl-fme
> rename Kconfig from INTEL_FPGA_FME to FPGA_DFL_FME
> v4: fix SPDX license issue, use dfl-fme as module name
> ---
> drivers/fpga/Kconfig | 10 +++
> drivers/fpga/Makefile | 3 +
> drivers/fpga/dfl-fme-main.c | 158 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 171 insertions(+)
> create mode 100644 drivers/fpga/dfl-fme-main.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 87f3d44..103d5e2 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -140,6 +140,16 @@ config FPGA_DFL
> Gate Array (FPGA) solutions which implement Device Feature List.
> It provides enumeration APIs, and feature device infrastructure.
>
> +config FPGA_DFL_FME
> + tristate "FPGA DFL FME Driver"
> + depends on FPGA_DFL
> + help
> + The FPGA Management Engine (FME) is a feature device implemented
> + under Device Feature List (DFL) framework. Select this option to
> + enable the platform device driver for FME which implements all
> + FPGA platform level management features. There shall be 1 FME
> + per DFL based FPGA device.
> +
> config FPGA_DFL_PCI
> tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 4375630..fbd1c85 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -30,6 +30,9 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
>
> # FPGA Device Feature List Support
> obj-$(CONFIG_FPGA_DFL) += dfl.o
> +obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> +
> +dfl-fme-objs := dfl-fme-main.o
>
> # Drivers for FPGAs which implement DFL
> obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> new file mode 100644
> index 0000000..ebe6b52
> --- /dev/null
> +++ b/drivers/fpga/dfl-fme-main.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA Management Engine (FME)
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Kang Luwei <luwei.kang@intel.com>
> + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + * Joseph Grecco <joe.grecco@intel.com>
> + * Enno Luebbers <enno.luebbers@intel.com>
> + * Tim Whisonant <tim.whisonant@intel.com>
> + * Ananda Ravuri <ananda.ravuri@intel.com>
> + * Henry Mitchel <henry.mitchel@intel.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include "dfl.h"
> +
> +static int fme_hdr_init(struct platform_device *pdev, struct feature *feature)
> +{
> + dev_dbg(&pdev->dev, "FME HDR Init.\n");
> +
> + return 0;
> +}
> +
> +static void fme_hdr_uinit(struct platform_device *pdev, struct feature *feature)
> +{
> + dev_dbg(&pdev->dev, "FME HDR UInit.\n");
> +}
> +
> +static const struct feature_ops fme_hdr_ops = {
> + .init = fme_hdr_init,
> + .uinit = fme_hdr_uinit,
> +};
> +
> +static struct feature_driver fme_feature_drvs[] = {
> + {
> + .id = FME_FEATURE_ID_HEADER,
> + .ops = &fme_hdr_ops,
> + },
> + {
> + .ops = NULL,
> + },
> +};
> +
> +static int fme_open(struct inode *inode, struct file *filp)
> +{
> + struct platform_device *fdev = fpga_inode_to_feature_dev(inode);
> + struct feature_platform_data *pdata = dev_get_platdata(&fdev->dev);
> + int ret;
> +
> + if (WARN_ON(!pdata))
> + return -ENODEV;
> +
> + ret = feature_dev_use_begin(pdata);
> + if (ret)
> + return ret;
> +
> + dev_dbg(&fdev->dev, "Device File Open\n");
> + filp->private_data = pdata;
> +
> + return 0;
> +}
> +
> +static int fme_release(struct inode *inode, struct file *filp)
> +{
> + struct feature_platform_data *pdata = filp->private_data;
> + struct platform_device *pdev = pdata->dev;
> +
> + dev_dbg(&pdev->dev, "Device File Release\n");
> + feature_dev_use_end(pdata);
> +
> + return 0;
> +}
> +
> +static long fme_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> + struct feature_platform_data *pdata = filp->private_data;
> + struct platform_device *pdev = pdata->dev;
> + struct feature *f;
> + long ret;
> +
> + dev_dbg(&pdev->dev, "%s cmd 0x%x\n", __func__, cmd);
> +
> + switch (cmd) {
> + default:
> + /*
> + * Let sub-feature's ioctl function to handle the cmd
> + * Sub-feature's ioctl returns -ENODEV when cmd is not
> + * handled in this sub feature, and returns 0 and other
> + * error code if cmd is handled.
> + */
> + fpga_dev_for_each_feature(pdata, f) {
> + if (f->ops && f->ops->ioctl) {
> + ret = f->ops->ioctl(pdev, f, cmd, arg);
> + if (ret == -ENODEV)
> + continue;
> + else
> + return ret;
continue and else aren't needed. Could be
if (ret != -ENODEV)
return ret;
Alan
> + }
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct file_operations fme_fops = {
> + .owner = THIS_MODULE,
> + .open = fme_open,
> + .release = fme_release,
> + .unlocked_ioctl = fme_ioctl,
> +};
> +
> +static int fme_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + ret = fpga_dev_feature_init(pdev, fme_feature_drvs);
> + if (ret)
> + goto exit;
> +
> + ret = fpga_register_dev_ops(pdev, &fme_fops, THIS_MODULE);
> + if (ret)
> + goto feature_uinit;
> +
> + return 0;
> +
> +feature_uinit:
> + fpga_dev_feature_uinit(pdev);
> +exit:
> + return ret;
> +}
> +
> +static int fme_remove(struct platform_device *pdev)
> +{
> + fpga_dev_feature_uinit(pdev);
> + fpga_unregister_dev_ops(pdev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver fme_driver = {
> + .driver = {
> + .name = FPGA_FEATURE_DEV_FME,
> + },
> + .probe = fme_probe,
> + .remove = fme_remove,
> +};
> +
> +module_platform_driver(fme_driver);
> +
> +MODULE_DESCRIPTION("FPGA Management Engine driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dfl-fme");
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH v4 20/24] fpga: dfl: add FPGA Accelerated Function Unit driver basic framework
From: Alan Tull @ 2018-04-05 18:26 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1518513893-4719-21-git-send-email-hao.wu@intel.com>
On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
One minor thing below.
> On DFL FPGA devices, the Accelerated Function Unit (AFU), can be
> reprogrammed for different functions. It connects to the FPGA
> infrastructure("blue bistream") via a Port. Port CSRs are implemented
> separately from the AFU CSRs to provide control and status of the Port.
> Once valid green bitstream is programmed into the AFU, it allows access
> to the AFU CSRs in the AFU MMIO space.
>
> This patch only implements basic driver framework for AFU, including
> device file operation framework.
>
> Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> ---
> v3: rename driver to dfl-afu-main.c
> v4: rename to dfl-port and fix SPDX license issue.
> ---
> drivers/fpga/Kconfig | 9 +++
> drivers/fpga/Makefile | 2 +
> drivers/fpga/dfl-afu-main.c | 159 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 170 insertions(+)
> create mode 100644 drivers/fpga/dfl-afu-main.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 65d54a4..4c6b45f 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -168,6 +168,15 @@ config FPGA_DFL_FME_REGION
> help
> Say Y to enable FPGA Region driver for FPGA Management Engine.
>
> +config FPGA_DFL_AFU
> + tristate "FPGA DFL AFU Driver"
> + depends on FPGA_DFL
> + help
> + This is the driver for FPGA Accelerated Function Unit (AFU) which
> + implements AFU and Port management features. A User AFU connects
> + to the FPGA infrastructure via a Port. There may be more than 1
> + Port/AFU per DFL based FPGA device.
> +
> config FPGA_DFL_PCI
> tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 163894e..5c9607b 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -34,8 +34,10 @@ obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> obj-$(CONFIG_FPGA_DFL_FME_MGR) += dfl-fme-mgr.o
> obj-$(CONFIG_FPGA_DFL_FME_BRIDGE) += dfl-fme-br.o
> obj-$(CONFIG_FPGA_DFL_FME_REGION) += dfl-fme-region.o
> +obj-$(CONFIG_FPGA_DFL_AFU) += dfl-afu.o
>
> dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
> +dfl-afu-objs := dfl-afu-main.o
>
> # Drivers for FPGAs which implement DFL
> obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> new file mode 100644
> index 0000000..70db28c
> --- /dev/null
> +++ b/drivers/fpga/dfl-afu-main.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA Accelerated Function Unit (AFU)
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Wu Hao <hao.wu@intel.com>
> + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + * Joseph Grecco <joe.grecco@intel.com>
> + * Enno Luebbers <enno.luebbers@intel.com>
> + * Tim Whisonant <tim.whisonant@intel.com>
> + * Ananda Ravuri <ananda.ravuri@intel.com>
> + * Henry Mitchel <henry.mitchel@intel.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include "dfl.h"
> +
> +static int port_hdr_init(struct platform_device *pdev, struct feature *feature)
> +{
> + dev_dbg(&pdev->dev, "PORT HDR Init.\n");
> +
> + return 0;
> +}
> +
> +static void port_hdr_uinit(struct platform_device *pdev,
> + struct feature *feature)
> +{
> + dev_dbg(&pdev->dev, "PORT HDR UInit.\n");
> +}
> +
> +static const struct feature_ops port_hdr_ops = {
> + .init = port_hdr_init,
> + .uinit = port_hdr_uinit,
> +};
> +
> +static struct feature_driver port_feature_drvs[] = {
> + {
> + .id = PORT_FEATURE_ID_HEADER,
> + .ops = &port_hdr_ops,
> + },
> + {
> + .ops = NULL,
> + }
> +};
> +
> +static int afu_open(struct inode *inode, struct file *filp)
> +{
> + struct platform_device *fdev = fpga_inode_to_feature_dev(inode);
> + struct feature_platform_data *pdata;
> + int ret;
> +
> + pdata = dev_get_platdata(&fdev->dev);
> + if (WARN_ON(!pdata))
> + return -ENODEV;
> +
> + ret = feature_dev_use_begin(pdata);
> + if (ret)
> + return ret;
> +
> + dev_dbg(&fdev->dev, "Device File Open\n");
> + filp->private_data = fdev;
> +
> + return 0;
> +}
> +
> +static int afu_release(struct inode *inode, struct file *filp)
> +{
> + struct platform_device *pdev = filp->private_data;
> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> + dev_dbg(&pdev->dev, "Device File Release\n");
> +
> + feature_dev_use_end(pdata);
> +
> + return 0;
> +}
> +
> +static long afu_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> + struct platform_device *pdev = filp->private_data;
> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct feature *f;
> + long ret;
> +
> + dev_dbg(&pdev->dev, "%s cmd 0x%x\n", __func__, cmd);
> +
> + switch (cmd) {
> + default:
> + /*
> + * Let sub-feature's ioctl function to handle the cmd
> + * Sub-feature's ioctl returns -ENODEV when cmd is not
> + * handled in this sub feature, and returns 0 and other
> + * error code if cmd is handled.
> + */
> + fpga_dev_for_each_feature(pdata, f)
> + if (f->ops && f->ops->ioctl) {
> + ret = f->ops->ioctl(pdev, f, cmd, arg);
> + if (ret == -ENODEV)
> + continue;
> + else
> + return ret;
The continue..else isn't needed. Could just be
if (ret != -ENODEV)
return ret;
Thanks,
Alan
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct file_operations afu_fops = {
> + .owner = THIS_MODULE,
> + .open = afu_open,
> + .release = afu_release,
> + .unlocked_ioctl = afu_ioctl,
> +};
> +
> +static int afu_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + dev_dbg(&pdev->dev, "%s\n", __func__);
> +
> + ret = fpga_dev_feature_init(pdev, port_feature_drvs);
> + if (ret)
> + return ret;
> +
> + ret = fpga_register_dev_ops(pdev, &afu_fops, THIS_MODULE);
> + if (ret)
> + fpga_dev_feature_uinit(pdev);
> +
> + return ret;
> +}
> +
> +static int afu_remove(struct platform_device *pdev)
> +{
> + dev_dbg(&pdev->dev, "%s\n", __func__);
> +
> + fpga_dev_feature_uinit(pdev);
> + fpga_unregister_dev_ops(pdev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver afu_driver = {
> + .driver = {
> + .name = FPGA_FEATURE_DEV_PORT,
> + },
> + .probe = afu_probe,
> + .remove = afu_remove,
> +};
> +
> +module_platform_driver(afu_driver);
> +
> +MODULE_DESCRIPTION("FPGA Accelerated Function Unit driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dfl-port");
> --
> 2.7.4
>
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Matthew Garrett @ 2018-04-05 18:03 UTC (permalink / raw)
To: Alan Cox
Cc: Linus Torvalds, luto, David Howells, Ard Biesheuvel, jmorris,
Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes, linux-man,
jlee, LSM List, linux-api, Kees Cook, linux-efi
In-Reply-To: <20180405185949.309216bb@alans-desktop>
On Thu, Apr 5, 2018 at 10:59 AM Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> VT-D
Once Intel provide that on all hardware and actually make it work reliably
with their graphics chipsets it's certainly a solution for the PCI DMA
problem, but right now it's still effectively undeployable for a lot of
real world cases.
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Alan Cox @ 2018-04-05 17:59 UTC (permalink / raw)
To: Matthew Garrett
Cc: Linus Torvalds, luto, David Howells, Ard Biesheuvel, jmorris,
Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes, linux-man,
jlee, LSM List, linux-api, Kees Cook, linux-efi
In-Reply-To: <CACdnJutOAPTmPm6dmHiw4+8rj=M7B1SiCkPaRZLmi9-CCuRyNg@mail.gmail.com>
> How? When there are random DMA-capable PCI devices that are driven by
> userland tools that are mmap()ing the BARs out of sysfs, how do we
> simultaneously avoid breaking those devices while also preventing the
> majority of users from being vulnerable to an attacker just DMAing over the
> kernel?
VT-D
^ permalink raw reply
* Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
From: jlee @ 2018-04-05 16:11 UTC (permalink / raw)
To: Mimi Zohar
Cc: David Howells, Andy Lutomirski, Greg Kroah-Hartman,
Theodore Y. Ts'o, Matthew Garrett, Linus Torvalds,
Ard Biesheuvel, James Morris, Alan Cox, Linux Kernel Mailing List,
Justin Forbes, linux-man, LSM List, Linux API, Kees Cook,
linux-efi
In-Reply-To: <1522936869.16421.63.camel@linux.vnet.ibm.com>
Hi Mimi,
On Thu, Apr 05, 2018 at 10:01:09AM -0400, Mimi Zohar wrote:
> On Thu, 2018-04-05 at 10:16 +0800, joeyli wrote:
> > Hi David,
> >
> > On Wed, Apr 04, 2018 at 05:17:24PM +0100, David Howells wrote:
> > > Andy Lutomirski <luto@kernel.org> wrote:
> > >
> > > > Since this thread has devolved horribly, I'm going to propose a solution.
> > > >
> > > > 1. Split the "lockdown" state into three levels: (please don't
> > > > bikeshed about the names right now.)
> > > >
> > > > LOCKDOWN_NONE: normal behavior
> > > >
> > > > LOCKDOWN_PROTECT_INTEGREITY: kernel tries to keep root from writing to
> > > > kernel memory
> > > >
> > > > LOCKDOWN_PROTECT_INTEGRITY_AND_SECRECY: kernel tries to keep root from
> > > > reading or writing kernel memory.
> > >
> > > In theory, it's good idea, but in practice it's not as easy to implement as I
> > > think you think.
> > >
> > > Let me list here the things that currently get restricted by lockdown:
> > >
> > [...snip]
> > > (5) Kexec.
> > >
> >
> > About IMA with kernel module signing and kexec(not on x86_64 yet)...
>
> Only carrying the measurement list across kexec is architecture
> specific, but everything else should work.
>
> > Because IMA can be used to verify the integrity of kernel module or even
> > the image for kexec. I think that the
> > IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY must be enabled at runtime
> > when kernel is locked-down.
>
> I think we need to understand the problem a bit better. Is the
> problem that you're using the secondary keyring and loading the UEFI
> keys onto the secondary keyring?
>
Sorry for my mistake. I want to write INTEGRITY_TRUSTED_KEYRING in
above but not IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY.
My brain is not clear when writing the mail.
> > Because the root can enroll master key to keyring then IMA trusts the ima key
> > derived from master key. It causes that the arbitrary signed module can be loaded
> > when the root compromised.
>
> With only the builtin keyring, only keys signed by a builtin key can
> be added to the IMA keyring.
>
Thanks for your description. I saw that the IMA_LOAD_X509 already depends
on IMA_TRUSTED_KEYRING (INTEGRITY_TRUSTED_KEYRING). Please ignore my concern.
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Alan Cox @ 2018-04-05 14:58 UTC (permalink / raw)
To: Matthew Garrett
Cc: Linus Torvalds, luto, David Howells, Ard Biesheuvel, jmorris,
Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes, linux-man,
jlee, LSM List, linux-api, Kees Cook, linux-efi
In-Reply-To: <CACdnJuvuf13VrHfBadvXvVLUQoqjdbwqmnsRHamfDN_O4Ve+kA@mail.gmail.com>
On Wed, 04 Apr 2018 00:12:04 +0000
Matthew Garrett <mjg59@google.com> wrote:
> On Tue, Apr 3, 2018 at 5:08 PM Linus Torvalds
> <torvalds@linux-foundation.org>
> wrote:
> > Still better than telling them to disable/enable secure boot, which
> > they may or may not even be able to to.
>
> Users who can boot a non-vendor Linux distribution on their platform can
> disable Secure Boot 100% of the time.
So can anyone else, or ignore it. Vendors of all OS's have released
enough buggy but signed kernel images over the past years that rummaging
around in the archive will find you a wide choice of signed boot images
that'll then let you do wtf you like including chaining some other target.
It was IMHO broken by design, it's always been broken by design and the
horse left the stable several years ago. Key revocation is hard, nobody
ever gets it right.
Thus "secure" boot is irrelevant to all of this
The most useful application of this kind of hardening is against remote
attacks. I don't care too much that someone local can attack my machine.
They can also steal it, ask me nicely with a baseball bat to remember the
password and so on.
If my box boots a random unsigned image that has these kinds of hardening
enabled then by the time it's on a network it's much much trickier to
attack. Yes you might be able to update the boot and reboot - but if I've
got that far I can insert an ancient buggy signed kernel image from a
vendor and chain through that anyway.
Real men boot security sensitive servers off a write protected SD card. If
your enterprise vendor doesn't supply a write protect for the boot
partition then maybe you should ask them why they don't 8)
In some ways the real application for this stuff is embedded. Whatever
the boot process most embedded devices benefit from that kind of lock
down.
Alan
^ permalink raw reply
* Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
From: Mimi Zohar @ 2018-04-05 14:01 UTC (permalink / raw)
To: joeyli, David Howells
Cc: Andy Lutomirski, Greg Kroah-Hartman, Theodore Y. Ts'o,
Matthew Garrett, Linus Torvalds, Ard Biesheuvel, James Morris,
Alan Cox, Linux Kernel Mailing List, Justin Forbes, linux-man,
LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <20180405021650.GC7362@linux-l9pv.suse>
On Thu, 2018-04-05 at 10:16 +0800, joeyli wrote:
> Hi David,
>
> On Wed, Apr 04, 2018 at 05:17:24PM +0100, David Howells wrote:
> > Andy Lutomirski <luto@kernel.org> wrote:
> >
> > > Since this thread has devolved horribly, I'm going to propose a solution.
> > >
> > > 1. Split the "lockdown" state into three levels: (please don't
> > > bikeshed about the names right now.)
> > >
> > > LOCKDOWN_NONE: normal behavior
> > >
> > > LOCKDOWN_PROTECT_INTEGREITY: kernel tries to keep root from writing to
> > > kernel memory
> > >
> > > LOCKDOWN_PROTECT_INTEGRITY_AND_SECRECY: kernel tries to keep root from
> > > reading or writing kernel memory.
> >
> > In theory, it's good idea, but in practice it's not as easy to implement as I
> > think you think.
> >
> > Let me list here the things that currently get restricted by lockdown:
> >
> [...snip]
> > (5) Kexec.
> >
>
> About IMA with kernel module signing and kexec(not on x86_64 yet)...
Only carrying the measurement list across kexec is architecture
specific, but everything else should work.
> Because IMA can be used to verify the integrity of kernel module or even
> the image for kexec. I think that the
> IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY must be enabled at runtime
> when kernel is locked-down.
I think we need to understand the problem a bit better. Is the
problem that you're using the secondary keyring and loading the UEFI
keys onto the secondary keyring?
> Because the root can enroll master key to keyring then IMA trusts the ima key
> derived from master key. It causes that the arbitrary signed module can be loaded
> when the root compromised.
With only the builtin keyring, only keys signed by a builtin key can
be added to the IMA keyring.
Mimi
^ permalink raw reply
* Re: [PATCH RFC v5] pidns: introduce syscall translate_pid
From: Konstantin Khlebnikov @ 2018-04-05 7:02 UTC (permalink / raw)
To: Eric W. Biederman, Nagarathnam Muthusamy
Cc: linux-api, linux-kernel, Jann Horn, Serge Hallyn, Oleg Nesterov,
Andy Lutomirski, Prakash Sangappa, Andrew Morton
In-Reply-To: <87h8oqhagl.fsf@xmission.com>
On 05.04.2018 01:29, Eric W. Biederman wrote:
> Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com> writes:
>
>> On 04/04/2018 12:11 PM, Konstantin Khlebnikov wrote:
>>> Each process have different pids, one for each pid namespace it belongs.
>>> When interaction happens within single pid-ns translation isn't required.
>>> More complicated scenarios needs special handling.
>>>
>>> For example:
>>> - reading pid-files or logs written inside container with pid namespace
>>> - attaching with ptrace to tasks from different pid namespace
>>> - passing pids across pid namespaces in any kind of API
>>>
>>> Currently there are several interfaces that could be used here:
>>>
>>> Pid namespaces are identified by inode number of /proc/[pid]/ns/pid.
>
> Using the inode number in interfaces is not an option. Especially not
> withou referencing the device number for the filesystem as well.
This is supposed to be single-instance fs,
not part of proc but referenced but its magic "symlinks".
Device numbers are not mentioned in "man namespaces".
>
>>> Pids for nested Pid namespaces are shown in file /proc/[pid]/status.
>>> In some cases conversion pid -> vpid could be easily done using this
>>> information, but backward translation requires scanning all tasks.
>>>
>>> Unix socket automatically translates pid attached to SCM_CREDENTIALS.
>>> This requires CAP_SYS_ADMIN for sending arbitrary pids and entering
>>> into pid namespace, this expose process and could be insecure.
>>>
>>> This patch adds new syscall for converting pids between pid namespaces:
>>>
>>> pid_t translate_pid(pid_t pid, int source_type, int source,
>>> int target_type, int target);
>>>
>>> @source_type and @target_type defines type of following arguments:
>>>
>>> TRANSLATE_PID_CURRENT_PIDNS - current pid namespace, argument is unused
>>> TRANSLATE_PID_TASK_PIDNS - task pid-ns, argument is task pid
>>
>> I believe using pid to represent the namespace has been already
>> discussed in V1 of this patch in https://lkml.org/lkml/2015/9/22/1087
>> after which we moved on to fd based version of this interface.
>
> Or in short why is the case of pids important?
>
> You Konstantin you almost said why they were important in your message
> saying you were going to send this one. However you don't explain in
> your description why you want to identify pid namespaces by pid.
>
Open of /proc/[pid]/ns/pid requires same permissions as ptrace,
pid based variant doesn't have such restrictions.
Most pid-based syscalls are racy in some cases but they are
here for decades and everybody knowns how to deal with it.
So, I've decided to merge both worlds in one interface which clearly tells what to expect.
^ permalink raw reply
* Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
From: joeyli @ 2018-04-05 2:16 UTC (permalink / raw)
To: David Howells
Cc: Andy Lutomirski, Greg Kroah-Hartman, Theodore Y. Ts'o,
Matthew Garrett, Linus Torvalds, Ard Biesheuvel, James Morris,
Alan Cox, Linux Kernel Mailing List, Justin Forbes, linux-man,
LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <1119.1522858644@warthog.procyon.org.uk>
Hi David,
On Wed, Apr 04, 2018 at 05:17:24PM +0100, David Howells wrote:
> Andy Lutomirski <luto@kernel.org> wrote:
>
> > Since this thread has devolved horribly, I'm going to propose a solution.
> >
> > 1. Split the "lockdown" state into three levels: (please don't
> > bikeshed about the names right now.)
> >
> > LOCKDOWN_NONE: normal behavior
> >
> > LOCKDOWN_PROTECT_INTEGREITY: kernel tries to keep root from writing to
> > kernel memory
> >
> > LOCKDOWN_PROTECT_INTEGRITY_AND_SECRECY: kernel tries to keep root from
> > reading or writing kernel memory.
>
> In theory, it's good idea, but in practice it's not as easy to implement as I
> think you think.
>
> Let me list here the things that currently get restricted by lockdown:
>
[...snip]
> (5) Kexec.
>
About IMA with kernel module signing and kexec(not on x86_64 yet)...
Because IMA can be used to verify the integrity of kernel module or even
the image for kexec. I think that the
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY must be enabled at runtime
when kernel is locked-down.
Because the root can enroll master key to keyring then IMA trusts the ima key
derived from master key. It causes that the arbitrary signed module can be loaded
when the root compromised.
Thanks
Joey Lee
^ permalink raw reply
* Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
From: joeyli @ 2018-04-05 1:48 UTC (permalink / raw)
To: David Howells
Cc: Jann Horn, Alexei Starovoitov, Andy Lutomirski,
Greg Kroah-Hartman, Theodore Y. Ts'o, Matthew Garrett,
Linus Torvalds, Ard Biesheuvel, James Morris, Alan Cox,
Linux Kernel Mailing List, Justin Forbes, linux-man, LSM List,
Linux API, Kees Cook, linux-efi
In-Reply-To: <15406.1522880367@warthog.procyon.org.uk>
On Wed, Apr 04, 2018 at 11:19:27PM +0100, David Howells wrote:
> Jann Horn <jannh@google.com> wrote:
>
> > > Uh, no. bpf, for example, can be used to modify kernel memory.
> >
> > I'm pretty sure bpf isn't supposed to be able to modify arbitrary
> > kernel memory. AFAIU if you can use BPF to write to arbitrary kernel
> > memory, that's a bug; with CAP_SYS_ADMIN, you can read from userspace,
> > write to userspace, and read from kernelspace, but you shouldn't be
> > able to write to kernelspace.
>
> Ah - you may be right. I seem to have misremembered what Joey Lee wrote in
> his patch description.
>
Sorry for it's my fault to misunderstood the behavoir of bpf with
CAP_SYS_ADMIN.
Joey Lee
^ permalink raw reply
* Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
From: joeyli @ 2018-04-05 1:45 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Greg Kroah-Hartman, Theodore Y. Ts'o, Matthew Garrett,
Linus Torvalds, David Howells, Ard Biesheuvel, James Morris,
Alan Cox, Linux Kernel Mailing List, Justin Forbes, linux-man,
LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <CALCETrUVQLDC6_VEvuAbdCOuGrmcxohqTB6P8eyNojm9AryNkg@mail.gmail.com>
Hi Andy,
On Wed, Apr 04, 2018 at 07:49:12AM -0700, Andy Lutomirski wrote:
> Since this thread has devolved horribly, I'm going to propose a solution.
...
> 6. There's a way to *decrease* the lockdown level below the configured
> value. (This ability itself may be gated by a config option.)
> Choices include a UEFI protected variable, an authenticated flag
> passed by the bootloader, and even just some special flag in the boot
> handoff protocol. It would be really quite useful for a user to be
> able to ask their bootloader to reduce the lockdown level for the
> purpose of a particular boot for debugging. I read the docs on
The "mokutil --disable-validation" done a similar bahvior as above.
Just it lets kernel to ignore the secure boot.
> mokutil --disable-validation, and it's quite messy. Let's have a way
> to do this that is mostly independent of the particular firmware in
> use.
>
Why the disabl-validation is messy?
The mokutil is shim specific but not dependent on particular firmware.
> I can imagine a grub option that decreases lockdown level along with a
> rule that grub will *not* load that option from its config, for
> example.
>
The root can modify the grub config to decrease lockdown level in next
boot without physcial accessing. The mokutil's interactive UI is used
to deal with user to confirm the physcial accessing.
Thanks
Joey Lee
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Peter Dolding @ 2018-04-05 1:28 UTC (permalink / raw)
To: Matthew Garrett
Cc: Linus Torvalds, luto, David Howells, Ard Biesheuvel, James Morris,
Alan Cox, Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes,
linux-man, jlee, LSM List, linux-api, Kees Cook, linux-efi
In-Reply-To: <CACdnJuuXgpnkMw8oacgadJQL5nE897cw3-RZBqV7df+19NuiMQ@mail.gmail.com>
On Thu, Apr 5, 2018 at 2:26 AM, Matthew Garrett <mjg59@google.com> wrote:
> On Tue, Apr 3, 2018 at 11:56 PM Peter Dolding <oiaohm@gmail.com> wrote:
>> On Wed, Apr 4, 2018 at 11:13 AM, Matthew Garrett <mjg59@google.com> wrote:
>
>> > There are four cases:
>> >
>> > Verified Boot off, lockdown off: Status quo in distro and mainline
> kernels
>> > Verified Boot off, lockdown on: Perception of security improvement
> that's
>> > trivially circumvented (and so bad)
>> > Verified Boot on, lockdown off: Perception of security improvement
> that's
>> > trivially circumvented (and so bad), status quo in mainline kernels
>> > Verified Boot on, lockdown on: Security improvement, status quo in
> distro
>> > kernels
>> >
>> > Of these four options, only two make sense. The most common
> implementation
>> > of Verified Boot on x86 platforms is UEFI Secure Boot,
>
>> Stop right there. Verified boot does not have to be UEFI secureboot.
>> You could be using a uboot verified boot or
>> https://www.coreboot.org/git-docs/Intel/vboot.html google vboot.
>> Neither of these provide flags to kernel to say they have been
>> performed.
>
> They can be modified to set the appropriate bit in the bootparams - the
> reason we can't do that in the UEFI case is that Linux can be built as a
> UEFI binary that the firmware execute directly, and so the firmware has no
> way to set that flag.
>
With some of your embedded hardware boot loaders you have exactly the
same problem. Where you cannot set bootparams instead have to hard
set everything in the kernel image. This is why there is a option to
embedded initramfs image inside kernel image because some of them will
only load 1 file.
So not using UEFI you run into the exact same problem. So lockdown
on or off need to be a kernel build option setting default. This
could be 3 options Always on, Always off and "Automatic based on boot
verification system status".
https://linux.die.net/man/8/efibootmgr
Also I have a problem here in non broken UEFI implementations -@ |
--append-binary-args that is very simple set the command line passed
into UEFI binary loaded by the firmware with the Linux kernel this
comes bootparams. Yes using --append-binary-args can be a pain it is
used to tell the Linux kernel where to find the / drive. So turning
lockdown off by bootparams is down right possible with working UEFI.
There is a lot of EFI out there that does not work properly.
>> Now Verified Boot on, lockdown off. Insanely this can be required in
>> diagnostic on some embedded platform because EFI secureboot does not
>> have a off switch. These are platforms where they don't boot if
>> they don't have a PK and KEK set installed. Yes some of these is jtag
>> the PK and KEK set in.
>
>> The fact that this Verified Boot on, lockdown off causes trouble
>> points to a clear problem. User owns the hardware they should have
>> the right to defeat secureboot if they wish to.
>
> Which is why Shim allows you to disable validation if you prove physical
> user presence.
Good idea until you have a motherboard where the PS2 ports have failed
and does not support usb keyboard so you have no keyboard until after
the kernel has booted so no way to prove physical presence. Or are
working on something embedded that has no physical user presence
interface in the boot stages these embedded devices can also be UEFI
with secureboot. Not everything running UEFI has keyboard,
screen....anything that you can prove physical user presence with
sometimes you have to pure depend on the signing key.
If I am a person who has made my own PK and has my own KEK in UEFI
system I should have the right to sign kernel with lockdown off by
default. I may need this for diagnostics on hardware without user
interface and I may need this because the hardware is broken and I
have set PK and KEK set by direct firmware flash access possibly by
jtag or possibly before critical port on motherboard died.
Of course I am not saying that Microsoft and others cannot have rules
that say if using their KEK that you cannot do this. But if the
machine is my hardware and I have set my own PK and KEK set I do know
what I am doing and I should be allowed to compromise security if I
wish its my hardware. I should not have to custom hack to do it.
Of course I am not saying that the setting in Linux kernel
configuration system cannot have a big warning that you should not do
this unless you have no other valid option and I am not saying that
the kernel should not log/report if it see what appears to be a
questionable configuration like dmesg "SECURITY ISSUE: UEFI
secureboot detected enabled kernel built with lockdown disabled system
at risk of comprise". Something audit tools could check logs for. .
So a kernel booting secureboot with lockdown disabled in kernel
configuration is perfectly fine to log a message that this is the
case. Always forcing lockdown on because you see UEFI secureboot
will cause issues.
Broken hardware to get around a failed motherboard with UEFI
secureboot locked on you may wish to chain load though a kernel that
you have the means to sign to boot a different OS that is going to be
complex for you to sign due to on going updates.
In my eyes lockdown have a kernel configuration option with 5 options
setting the mode.
1) Automatic: On if a verified boot system is known to the kernel this
may be UEFI secure boot this could be other systems the Linux kernel
can detect in future and off if verified boot is not detected/disabled
or if user approves turning it off by user presence test.,
2) Always on: That no matter what the Linux kernel turns lockdown on.
No user action or shim action is going to turn it off.
3) On but user/boot bootparams controllable. So user can give
command line to EFI or other boot-loader to turn it off so no user
presence test and this is need in some cases..
4) Always off: that is always off no matter what and put error in
dmesg if the Automatic on condition is found.
5) Completely build linux kernel without lockdown code bar with a
single dmesg reporting this.
3-5 all provide security risks all have valid usage reasons.
3 you may want command line controllable you are developing on a
system with user presence cannot be confirmed and you need to be able
to switch between lockdown on and off and you only want to send 1
kernel cross to device in development stages.
4 is simple you have a case where you need lockdown always off line
chain loading on a system where things have gone wrong.
5 can quite simply be diagnostics we have a issue we want to
absolutely confirm that the lockdown code is not the cause.
I can understand if 3-5 are forbin when using Microsoft KEK or other
parties KEKs but those building their own kernels using their own keys
should not have those restrictions it their hardware it thier
configuration if they decide to make it insecure it their choice..
Peter Dolding
^ permalink raw reply
* Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
From: Matthew Garrett @ 2018-04-05 0:22 UTC (permalink / raw)
To: jmorris
Cc: David Howells, luto, Greg Kroah-Hartman, Theodore Ts'o,
Linus Torvalds, Ard Biesheuvel, Alan Cox,
Linux Kernel Mailing List, jforbes, linux-man, jlee, LSM List,
linux-api, Kees Cook, linux-efi
In-Reply-To: <alpine.LRH.2.21.1804050918330.21060@namei.org>
On Wed, Apr 4, 2018 at 4:25 PM James Morris <jmorris@namei.org> wrote:
> It's surely reasonable to allow an already secure-booted system to be
> debugged without needing to be rebooted.
alt-sysrq-x from a physical console will do that.
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Matthew Garrett @ 2018-04-05 0:20 UTC (permalink / raw)
To: Peter Dolding
Cc: Theodore Ts'o, David Howells, Linus Torvalds, luto,
Ard Biesheuvel, jmorris, Alan Cox, Greg Kroah-Hartman,
Linux Kernel Mailing List, jforbes, linux-man, jlee, LSM List,
linux-api, Kees Cook, linux-efi
In-Reply-To: <CANA3KFWLCfe=KYPBhKj+g698=frt4E-u-Hq2--5jFkHnkNoBnA@mail.gmail.com>
On Wed, Apr 4, 2018 at 5:05 PM Peter Dolding <oiaohm@gmail.com> wrote:
> > If you don't have secure boot then an attacker with root can modify your
> > bootloader or kernel, and on next boot lockdown can be silently
disabled.
> Stop being narrow minded you don't need secure boot to protect
> bootloader or kernel the classic is only boot from read only media.
And if you use another protected path you can set the appropriate bootparam
flag or pass the appropriate kernel command line argument and gain the same
functionality.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox