Re: [PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
Hi Kees,
On 2020-07-17 3:06 p.m., Kees Cook wrote:
On Fri, Jul 17, 2020 at 12:04:18PM -0700, Scott Branden wrote:
On 2020-07-17 10:43 a.m., Kees Cook wrote:
In preparation for refactoring kernel_read_file*(), remove the redundant
"size" argument which is not needed: it can be included in the return
I don't think the size argument is redundant though.
The existing kernel_read_file functions always read the whole file.
Now, what happens if the file is bigger than the buffer.
How does kernel_read_file know it read the whole file by looking at the
return value?
Yes; an entirely reasonable concern. This is why I add the file_size
output argument later in the series.
There is something wrong with this patch. I apply patches 1-5 and these
pass the kernel self test.
Patch 6 does not pass the kernel-selftest/firmware/fw_run_tests.sh
code, with callers adjusted. (VFS reads already cannot be larger than
INT_MAX.)
[...]
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
Should this be SSIZE_MAX?
No, for two reasons: then we need to change the return value and likely
the callers need more careful checks, and more importantly, because the
VFS already limits single read actions to INT_MAX, so limits above this
make no sense. Win win! :)
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 0/4] printk: reimplement LOG_CONT handling
On Fri, Jul 17, 2020 at 4:48 PM John Ogness wrote: > > Using placeholders avoids tools such as systemd-journald from > erroneously reporting missed messages. However, it also means that > empty placeholder records are visible in systemd-journald logs and > displayed in tools such as dmesg. As long as the readers still reliably do the joining, this is fine. HOWEVER. Make sure you test the case of "fast concurrent readers". The last time we did things like this, it was a disaster, because a concurrent reader would see and return the _incomplete_ line, and the next entry was still being generated on another CPU. The reader would then decide to return that incomplete line, because it had something. And while in theory this could then be handled properly in user space, in practice it wasn't. So you'd see a lot of logging tools that would then report all those continuations as separate log events. Which is the whole point of LOG_CONT - for that *not* to happen. So this is just a heads-up that I will not pull something that breaks LOG_CONT because it thinks "user space can handle it". No. User space does not handle it, and we need to handle it for the user. So as long as the kernel makes sure the joining does happen it at some point, it's all fine. It obviously doesn't have to happen at printk() time, just as long as incomplete records aren't exposed even to concurrent readers. A test-case with a short delay in between writes might be a good idea, although the last time this broke it was very clear in peoples syslogs and dmesg because it turns out there are lots of concurrent readers at boot time and _somebody_ will hit the race. Linus ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 1/4] printk: ringbuffer: support dataless records
In order to support storage of continuous lines, dataless records must
be allowed. For example, these are generated with the legal calls:
pr_info("");
pr_cont("\n");
Currently dataless records are denoted by INVALID_LPOS in order to
recognize failed prb_reserve() calls. Change the code to use two
different identifiers (FAILED_LPOS and NO_LPOS) to distinguish
between failed prb_reserve() records and successful dataless records.
Signed-off-by: John Ogness
---
kernel/printk/printk_ringbuffer.c | 58 ++-
kernel/printk/printk_ringbuffer.h | 15
2 files changed, 35 insertions(+), 38 deletions(-)
diff --git a/kernel/printk/printk_ringbuffer.c
b/kernel/printk/printk_ringbuffer.c
index 7355ca99e852..54b0a6324dbf 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -264,6 +264,9 @@
/* Determine how many times the data array has wrapped. */
#define DATA_WRAPS(data_ring, lpos)((lpos) >> (data_ring)->size_bits)
+/* Determine if a logical position refers to a data-less block. */
+#define LPOS_DATALESS(lpos)((lpos) & 1UL)
+
/* Get the logical position at index 0 of the current wrap. */
#define DATA_THIS_WRAP_START_LPOS(data_ring, lpos) \
((lpos) & ~DATA_SIZE_MASK(data_ring))
@@ -320,21 +323,13 @@ static unsigned int to_blk_size(unsigned int size)
* block does not exceed the maximum possible size that could fit within the
* ringbuffer. This function provides that basic size check so that the
* assumption is safe.
- *
- * Writers are also not allowed to write 0-sized (data-less) records. Such
- * records are used only internally by the ringbuffer.
*/
static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size)
{
struct prb_data_block *db = NULL;
- /*
-* Writers are not allowed to write data-less records. Such records
-* are used only internally by the ringbuffer to denote records where
-* their data failed to allocate or have been lost.
-*/
if (size == 0)
- return false;
+ return true;
/*
* Ensure the alignment padded size could possibly fit in the data
@@ -568,8 +563,8 @@ static bool data_push_tail(struct printk_ringbuffer *rb,
unsigned long tail_lpos;
unsigned long next_lpos;
- /* If @lpos is not valid, there is nothing to do. */
- if (lpos == INVALID_LPOS)
+ /* If @lpos is from a data-less block, there is nothing to do. */
+ if (LPOS_DATALESS(lpos))
return true;
/*
@@ -962,8 +957,8 @@ static char *data_alloc(struct printk_ringbuffer *rb,
if (size == 0) {
/* Specify a data-less block. */
- blk_lpos->begin = INVALID_LPOS;
- blk_lpos->next = INVALID_LPOS;
+ blk_lpos->begin = NO_LPOS;
+ blk_lpos->next = NO_LPOS;
return NULL;
}
@@ -976,8 +971,8 @@ static char *data_alloc(struct printk_ringbuffer *rb,
if (!data_push_tail(rb, data_ring, next_lpos -
DATA_SIZE(data_ring))) {
/* Failed to allocate, specify a data-less block. */
- blk_lpos->begin = INVALID_LPOS;
- blk_lpos->next = INVALID_LPOS;
+ blk_lpos->begin = FAILED_LPOS;
+ blk_lpos->next = FAILED_LPOS;
return NULL;
}
@@ -1025,6 +1020,10 @@ static char *data_alloc(struct printk_ringbuffer *rb,
static unsigned int space_used(struct prb_data_ring *data_ring,
struct prb_data_blk_lpos *blk_lpos)
{
+ /* Data-less blocks take no space. */
+ if (LPOS_DATALESS(blk_lpos->begin))
+ return 0;
+
if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring,
blk_lpos->next)) {
/* Data block does not wrap. */
return (DATA_INDEX(data_ring, blk_lpos->next) -
@@ -1080,11 +1079,8 @@ bool prb_reserve(struct prb_reserved_entry *e, struct
printk_ringbuffer *rb,
if (!data_check_size(>text_data_ring, r->text_buf_size))
goto fail;
- /* Records are allowed to not have dictionaries. */
- if (r->dict_buf_size) {
- if (!data_check_size(>dict_data_ring, r->dict_buf_size))
- goto fail;
- }
+ if (!data_check_size(>dict_data_ring, r->dict_buf_size))
+ goto fail;
/*
* Descriptors in the reserved state act as blockers to all further
@@ -1212,10 +1208,8 @@ static char *get_data(struct prb_data_ring *data_ring,
struct prb_data_block *db;
/* Data-less data block description. */
- if (blk_lpos->begin == INVALID_LPOS &&
- blk_lpos->next == INVALID_LPOS) {
+ if (LPOS_DATALESS(blk_lpos->begin) && LPOS_DATALESS(blk_lpos->next))
return NULL;
- }
[PATCH 2/4] printk: store instead of processing cont parts
Instead of buffering continuous line parts before storing the full
line into the ringbuffer, store each part as its own record.
Signed-off-by: John Ogness
---
kernel/printk/printk.c | 114 -
1 file changed, 11 insertions(+), 103 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fec71229169e..c4274c867771 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -495,9 +495,14 @@ static void truncate_msg(u16 *text_len, u16 *trunc_msg_len)
*trunc_msg_len = 0;
}
+static inline u32 printk_caller_id(void)
+{
+ return in_task() ? task_pid_nr(current) :
+ 0x8000 + raw_smp_processor_id();
+}
+
/* insert record into the buffer, discard old ones, update heads */
-static int log_store(u32 caller_id, int facility, int level,
-enum log_flags flags, u64 ts_nsec,
+static int log_store(int facility, int level, enum log_flags flags,
const char *dict, u16 dict_len,
const char *text, u16 text_len)
{
@@ -525,11 +530,8 @@ static int log_store(u32 caller_id, int facility, int
level,
r.info->facility = facility;
r.info->level = level & 7;
r.info->flags = flags & 0x1f;
- if (ts_nsec > 0)
- r.info->ts_nsec = ts_nsec;
- else
- r.info->ts_nsec = local_clock();
- r.info->caller_id = caller_id;
+ r.info->ts_nsec = local_clock();
+ r.info->caller_id = printk_caller_id();
/* insert message */
prb_commit();
@@ -1874,100 +1876,6 @@ static inline void printk_delay(void)
}
}
-static inline u32 printk_caller_id(void)
-{
- return in_task() ? task_pid_nr(current) :
- 0x8000 + raw_smp_processor_id();
-}
-
-/*
- * Continuation lines are buffered, and not committed to the record buffer
- * until the line is complete, or a race forces it. The line fragments
- * though, are printed immediately to the consoles to ensure everything has
- * reached the console in case of a kernel crash.
- */
-static struct cont {
- char buf[LOG_LINE_MAX];
- size_t len; /* length == 0 means unused buffer */
- u32 caller_id; /* printk_caller_id() of first print */
- u64 ts_nsec;/* time of first print */
- u8 level; /* log level of first message */
- u8 facility;/* log facility of first message */
- enum log_flags flags; /* prefix, newline flags */
-} cont;
-
-static void cont_flush(void)
-{
- if (cont.len == 0)
- return;
-
- log_store(cont.caller_id, cont.facility, cont.level, cont.flags,
- cont.ts_nsec, NULL, 0, cont.buf, cont.len);
- cont.len = 0;
-}
-
-static bool cont_add(u32 caller_id, int facility, int level,
-enum log_flags flags, const char *text, size_t len)
-{
- /* If the line gets too long, split it up in separate records. */
- if (cont.len + len > sizeof(cont.buf)) {
- cont_flush();
- return false;
- }
-
- if (!cont.len) {
- cont.facility = facility;
- cont.level = level;
- cont.caller_id = caller_id;
- cont.ts_nsec = local_clock();
- cont.flags = flags;
- }
-
- memcpy(cont.buf + cont.len, text, len);
- cont.len += len;
-
- // The original flags come from the first line,
- // but later continuations can add a newline.
- if (flags & LOG_NEWLINE) {
- cont.flags |= LOG_NEWLINE;
- cont_flush();
- }
-
- return true;
-}
-
-static size_t log_output(int facility, int level, enum log_flags lflags, const
char *dict, size_t dictlen, char *text, size_t text_len)
-{
- const u32 caller_id = printk_caller_id();
-
- /*
-* If an earlier line was buffered, and we're a continuation
-* write from the same context, try to add it to the buffer.
-*/
- if (cont.len) {
- if (cont.caller_id == caller_id && (lflags & LOG_CONT)) {
- if (cont_add(caller_id, facility, level, lflags, text,
text_len))
- return text_len;
- }
- /* Otherwise, make sure it's flushed */
- cont_flush();
- }
-
- /* Skip empty continuation lines that couldn't be added - they just
flush */
- if (!text_len && (lflags & LOG_CONT))
- return 0;
-
- /* If it doesn't end in a newline, try to buffer the current line */
- if (!(lflags & LOG_NEWLINE)) {
- if (cont_add(caller_id, facility, level, lflags, text,
text_len))
- return text_len;
- }
-
- /* Store it in the record log */
- return log_store(caller_id, facility, level, lflags, 0,
-
[PATCH 3/4] printk: process cont records during reading
Readers of the printk ringbuffer can use the continuous line interface
to read full lines. The interface buffers continuous line parts until
the full line is available or that line was interrupted by a writer
from another context.
The continuous line interface automatically throws out partial lines if
a reader jumps to older sequence numbers. If a reader jumps to higher
sequence numbers, any cached partial lines are flushed.
The continuous line interface is used by:
- console printing
- syslog
- devkmsg
devkmsg has the additional requirement that it must show a line for
every sequence number if the corresponding continuous line record was
not dropped. The continuous line interface supports this by allowing
the reader to provide a printk_record struct that will be filled in
with placeholder information (but no text) in case a full line is not
yet available.
Note that kmsg_dump does not use the continuous line interface.
The continuous line interface discards dictionaries of continuous lines.
Signed-off-by: John Ogness
---
kernel/printk/printk.c | 455 +
1 file changed, 371 insertions(+), 84 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c4274c867771..363ef290f313 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -657,6 +657,287 @@ static ssize_t msg_print_ext_body(char *buf, size_t size,
return p - buf;
}
+/*
+ * Readers of the printk ringbuffer can use the continuous line interface
+ * to read full lines. The interface buffers continuous line parts until
+ * the full line is available or that line was interrupted by a writer
+ * from another context.
+ *
+ * The continuous line interface automatically throws out partial lines if a
+ * reader jumps to older sequence numbers. If a reader jumps to higher
+ * sequence numbers, any cached partial lines are flushed.
+ *
+ * The continuous line interface is used by:
+ *
+ * - console printing
+ * - syslog
+ * - devkmsg
+ *
+ * devkmsg has the additional requirement that it must show a line for every
+ * sequence number if the corresponding continuous line record was not dropped.
+ * The continuous line interface supports this by allowing the reader to
+ * provide a printk_record struct that will be filled in with placeholder
+ * information (but no text) in case a full line is not yet available.
+ *
+ * Note that kmsg_dump does not use the continuous line interface.
+ *
+ * The continuous line interface discards dictionaries of continuous lines.
+ */
+
+struct cont_record {
+ struct printk_recordr;
+ struct printk_info info;
+ chartext[LOG_LINE_MAX + PREFIX_MAX];
+ boolset;
+};
+
+/*
+ * The continuous line buffer manager.
+ *
+ * @cr:record buffers for reading and caching continuous lines
+ * @dict: the dictionary used when reading a record
+ * @cache_ind: index of the cache record in @cr
+ * @begin_seq: the minimal sequence number of the current continuous line
+ * @end_seq: the maximal sequence number of the current continuous line
+ * @dropped: count of dropped records during the current continuous line
+ */
+struct cont {
+ struct cont_record cr[2];
+ chardict[LOG_LINE_MAX];
+ int cache_ind;
+ u64 begin_seq;
+ u64 end_seq;
+ unsigned long dropped;
+};
+
+/*
+ * Initialize the continuous line manager. As an alternative, it is also
+ * acceptable if the structure is set to all zeros.
+ */
+static void cont_init(struct cont *c, u64 seq)
+{
+ c->cr[0].set = false;
+ c->cr[1].set = false;
+ c->cache_ind = 0;
+ c->begin_seq = seq;
+ c->end_seq = seq;
+ c->dropped = 0;
+}
+
+/* Get the continuous line cache, if one exists. */
+static struct printk_record *cont_cache(struct cont *c)
+{
+ struct cont_record *cr = >cr[c->cache_ind];
+
+ if (!cr->set)
+ return NULL;
+ return >r;
+}
+
+/*
+ * Like cont_cache(), but also flushes the dropped count, clears the
+ * dictionary, and switches to the other record buffer for future caching.
+ */
+static struct printk_record *cont_flush(struct cont *c, unsigned long *dropped)
+{
+ struct cont_record *cr = >cr[c->cache_ind];
+
+ c->cache_ind ^= 1;
+
+ if (!cr->set)
+ return NULL;
+
+ if (dropped)
+ *dropped = c->dropped;
+ c->dropped = 0;
+
+ c->begin_seq = cr->info.seq;
+ cr->info.dict_len = 0;
+ cr->set = false;
+
+ return >r;
+}
+
+/*
+ * Wrapper for prb_read_valid() that reads a new record into the
+ * non-caching record buffer.
+ */
+static struct printk_record *cont_read(struct cont *c, u64 seq)
+{
+ struct cont_record *cr = >cr[c->cache_ind ^ 1];
+ struct printk_record *r = >r;
+
+ prb_rec_init_rd(r, >info, cr->text,
[PATCH 0/4] printk: reimplement LOG_CONT handling
Hello, Here is the second series to rework the printk subsystem. This series removes LOG_CONT handling from printk() callers, storing all LOG_CONT parts individually in the ringbuffer. With this series, LOG_CONT handling is moved to the ringbuffer readers that provide the record contents to users (console printing, syslog, /dev/kmsg). This change is necessary in order to support the upcoming move to a fully lockless printk() implementation. This series is in line with the agreements [0] made at the meeting during LPC2019 in Lisbon, with 1 exception: For the /dev/kmsg interface, empty line placeholder records are reported for the LOG_CONT parts. Using placeholders avoids tools such as systemd-journald from erroneously reporting missed messages. However, it also means that empty placeholder records are visible in systemd-journald logs and displayed in tools such as dmesg. The effect can be easily observed with the sysrq help: $ echo h | sudo tee /proc/sysrq-trigger $ sudo dmesg | tail -n 30 $ sudo journalctl -k -n 30 Providing the placeholder entries allows a userspace tool to identify if records were actually lost. IMHO this an important feature. Its side effect can be addressed by userspace tools if they change to silently consume empty records. For dump tools that process the ringbuffer directly (such as crash, makedumpfile, kexec-tools), they will need to implement LOG_CONT handling if they want to present clean continuous line messages. Finally, by moving LOG_CONT handling from writers to readers, some incorrect pr_cont() usage is revealed. Patch 4 of this series addresses one such example. This series is based on the printk git tree [1] printk-rework branch. [0] https://lkml.kernel.org/r/87k1acz5rx@linutronix.de [1] https://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git (printk-rework branch) John Ogness (4): printk: ringbuffer: support dataless records printk: store instead of processing cont parts printk: process cont records during reading ipconfig: cleanup printk usage kernel/printk/printk.c| 569 -- kernel/printk/printk_ringbuffer.c | 58 ++- kernel/printk/printk_ringbuffer.h | 15 +- net/ipv4/ipconfig.c | 25 +- 4 files changed, 434 insertions(+), 233 deletions(-) -- 2.20.1 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 4/4] ipconfig: cleanup printk usage
The use of pr_info() and pr_cont() was not ordered correctly for
all cases. Order it so that all cases provide the expected output.
Signed-off-by: John Ogness
---
net/ipv4/ipconfig.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index 561f15b5a944..0f4bd7a59310 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -1442,6 +1442,9 @@ static int __init ip_auto_config(void)
#endif
int err;
unsigned int i;
+#ifndef IPCONFIG_SILENT
+ bool pr0;
+#endif
/* Initialise all name servers and NTP servers to NONE (but only if the
* "ip=" or "nfsaddrs=" kernel command line parameters weren't decoded,
@@ -1575,31 +1578,37 @@ static int __init ip_auto_config(void)
if (ic_dev_mtu)
pr_cont(", mtu=%d", ic_dev_mtu);
/* Name servers (if any): */
+ pr0 = false;
for (i = 0; i < CONF_NAMESERVERS_MAX; i++) {
if (ic_nameservers[i] != NONE) {
- if (i == 0)
+ if (!pr0) {
pr_info(" nameserver%u=%pI4",
i, _nameservers[i]);
- else
+ pr0 = true;
+ } else {
pr_cont(", nameserver%u=%pI4",
i, _nameservers[i]);
+ }
}
- if (i + 1 == CONF_NAMESERVERS_MAX)
- pr_cont("\n");
}
+ if (pr0)
+ pr_cont("\n");
/* NTP servers (if any): */
+ pr0 = false;
for (i = 0; i < CONF_NTP_SERVERS_MAX; i++) {
if (ic_ntp_servers[i] != NONE) {
- if (i == 0)
+ if (!pr0) {
pr_info(" ntpserver%u=%pI4",
i, _ntp_servers[i]);
- else
+ pr0 = true;
+ } else {
pr_cont(", ntpserver%u=%pI4",
i, _ntp_servers[i]);
+ }
}
- if (i + 1 == CONF_NTP_SERVERS_MAX)
- pr_cont("\n");
}
+ if (pr0)
+ pr_cont("\n");
#endif /* !SILENT */
/*
--
2.20.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 00/13] Introduce partial kernel_read_file() support
On Fri, Jul 17, 2020 at 12:17:02PM -0700, Scott Branden wrote: > Thanks for sending out. This looks different than your other patch series. Yes, it mutated in my head as I considered how all of this should hang together, which is why I wanted to get it sent before the weekend. I'm still trying to figure out why the fireware testsuite fails for me, etc. > We should get the first 5 patches accepted now though as they are > simple cleanups and fixes. That will reduce the number of outstanding > patches in the series. Agreed. I'd like to get some more eyes on it, but I can get it ready for -next. > At first glance the issue with the changes after that is the existing > API assumes it has read the whole file and failed if it did not. > Now, if the file is larger than the amount requested there is no indication? The intention is to have old API users unchanged and new users can use a pre-allocated buf (with buf_size) along with file_size to examine their partial read progress. If I broke the old API, that's a bug and I need to fix it, but that's why I wanted to start with the firmware test suite (basic things like module loading work fine after this series, but I wanted to really exercise the corners that the firmware suite pokes at). -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
On Fri, Jul 17, 2020 at 12:04:18PM -0700, Scott Branden wrote:
> On 2020-07-17 10:43 a.m., Kees Cook wrote:
> > In preparation for refactoring kernel_read_file*(), remove the redundant
> > "size" argument which is not needed: it can be included in the return
>
> I don't think the size argument is redundant though.
> The existing kernel_read_file functions always read the whole file.
> Now, what happens if the file is bigger than the buffer.
> How does kernel_read_file know it read the whole file by looking at the
> return value?
Yes; an entirely reasonable concern. This is why I add the file_size
output argument later in the series.
> > code, with callers adjusted. (VFS reads already cannot be larger than
> > INT_MAX.)
> > [...]
> > - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
> > + if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
>
> Should this be SSIZE_MAX?
No, for two reasons: then we need to change the return value and likely
the callers need more careful checks, and more importantly, because the
VFS already limits single read actions to INT_MAX, so limits above this
make no sense. Win win! :)
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 03/12] powerpc/kexec_file: add helper functions for getting memory ranges
On 17/07/20 10:02 am, Hari Bathini wrote:
>
>
> On 15/07/20 5:19 am, Thiago Jung Bauermann wrote:
>>
>> Hello Hari,
>>
>> Hari Bathini writes:
>>
>>> In kexec case, the kernel to be loaded uses the same memory layout as
>>> the running kernel. So, passing on the DT of the running kernel would
>>> be good enough.
>>>
>>> But in case of kdump, different memory ranges are needed to manage
>>> loading the kdump kernel, booting into it and exporting the elfcore
>>> of the crashing kernel. The ranges are exlude memory ranges, usable
>>
>> s/exlude/exclude/
>>
>>> memory ranges, reserved memory ranges and crash memory ranges.
>>>
>>> Exclude memory ranges specify the list of memory ranges to avoid while
>>> loading kdump segments. Usable memory ranges list the memory ranges
>>> that could be used for booting kdump kernel. Reserved memory ranges
>>> list the memory regions for the loading kernel's reserve map. Crash
>>> memory ranges list the memory ranges to be exported as the crashing
>>> kernel's elfcore.
>>>
>>> Add helper functions for setting up the above mentioned memory ranges.
>>> This helpers facilitate in understanding the subsequent changes better
>>> and make it easy to setup the different memory ranges listed above, as
>>> and when appropriate.
>>>
>>> Signed-off-by: Hari Bathini
>>> Tested-by: Pingfan Liu
>>
>
>
>
>>> +/**
>>> + * add_reserved_ranges - Adds "/reserved-ranges" regions exported by f/w
>>> + * to the given memory ranges list.
>>> + * @mem_ranges: Range list to add the memory ranges to.
>>> + *
>>> + * Returns 0 on success, negative errno on error.
>>> + */
>>> +int add_reserved_ranges(struct crash_mem **mem_ranges)
>>> +{
>>> + int i, len, ret = 0;
>>> + const __be32 *prop;
>>> +
>>> + prop = of_get_property(of_root, "reserved-ranges", );
>>> + if (!prop)
>>> + return 0;
>>> +
>>> + /*
>>> +* Each reserved range is an (address,size) pair, 2 cells each,
>>> +* totalling 4 cells per range.
>>
>> Can you assume that, or do you need to check the #address-cells and
>> #size-cells properties of the root node?
>
> Taken from early_reserve_mem_dt() which did not seem to care.
> Should we be doing any different here?
On second thoughts, wouldn't hurt to be extra cautious. Will use
#address-cells & #size-cells to parse reserved-ranges.
Thanks
Hari
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
On 2020-07-17 12:04 p.m., Scott Branden wrote:
Hi Kees,
On 2020-07-17 10:43 a.m., Kees Cook wrote:
In preparation for refactoring kernel_read_file*(), remove the redundant
"size" argument which is not needed: it can be included in the return
I don't think the size argument is redundant though.
The existing kernel_read_file functions always read the whole file.
Now, what happens if the file is bigger than the buffer.
How does kernel_read_file know it read the whole file by looking at
the return value?
Actually, this change looks ok dealing with the size. I'll look at the
rest.
code, with callers adjusted. (VFS reads already cannot be larger than
INT_MAX.)
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 8
fs/kernel_read_file.c | 20 +---
include/linux/kernel_read_file.h | 8
kernel/kexec_file.c | 13 ++---
kernel/module.c | 7 +++
security/integrity/digsig.c | 5 +++--
security/integrity/ima/ima_fs.c | 5 +++--
7 files changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index d4a413ea48ce..ea419c7d3d34 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -462,7 +462,7 @@ fw_get_filesystem_firmware(struct device *device,
struct fw_priv *fw_priv,
size_t in_size,
const void *in_buffer))
{
- loff_t size;
+ size_t size;
int i, len;
int rc = -ENOENT;
char *path;
@@ -494,10 +494,9 @@ fw_get_filesystem_firmware(struct device
*device, struct fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, ,
- , msize,
+ rc = kernel_read_file_from_path_initns(path, , msize,
READING_FIRMWARE);
- if (rc) {
+ if (rc < 0) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error %d\n",
path, rc);
@@ -506,6 +505,7 @@ fw_get_filesystem_firmware(struct device *device,
struct fw_priv *fw_priv,
path);
continue;
}
+ size = rc;
dev_dbg(device, "Loading firmware from %s\n", path);
if (decompress) {
dev_dbg(device, "f/w decompressing %s\n",
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 54d972d4befc..dc28a8def597 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,7 +5,7 @@
#include
#include
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
+int kernel_read_file(struct file *file, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
@@ -29,7 +29,7 @@ int kernel_read_file(struct file *file, void **buf,
loff_t *size,
ret = -EINVAL;
goto out;
}
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
Should this be SSIZE_MAX?
ret = -EFBIG;
goto out;
}
@@ -59,8 +59,6 @@ int kernel_read_file(struct file *file, void **buf,
loff_t *size,
}
ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
out_free:
if (ret < 0) {
@@ -72,11 +70,11 @@ int kernel_read_file(struct file *file, void
**buf, loff_t *size,
out:
allow_write_access(file);
- return ret;
+ return ret == 0 ? pos : ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file);
-int kernel_read_file_from_path(const char *path, void **buf,
loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
struct file *file;
@@ -89,14 +87,14 @@ int kernel_read_file_from_path(const char *path,
void **buf, loff_t *size,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, max_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
+ loff_t max_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -115,13 +113,13 @@ int kernel_read_file_from_path_initns(const
char *path, void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, max_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-int
Re: [PATCH 00/13] Introduce partial kernel_read_file() support
Hi Kees, Thanks for sending out. This looks different than your other patch series. We should get the first 5 patches accepted now though as they are simple cleanups and fixes. That will reduce the number of outstanding patches in the series. At first glance the issue with the changes after that is the existing API assumes it has read the whole file and failed if it did not. Now, if the file is larger than the amount requested there is no indication? On 2020-07-17 10:42 a.m., Kees Cook wrote: Hi, Here's my attempt at clearing the path to partial read support in kernel_read_file(), which fixes a number of issues along the way. I'm still fighting with the firmware test suite (it doesn't seem to pass for me even in stock v5.7... ?) But I don't want to block Scott's work[1] any this week, so here's the series as it is currently. The primary difference to Scott's approach is to avoid adding a new set of functions and just adapt the existing APIs to deal with "offset". Also, the fixes for the enum are first in the series so they can be backported without the header file relocation. I'll keep poking at the firmware tests... -Kees [1] https://lore.kernel.org/lkml/202007161415.10D015477@keescook/ Kees Cook (12): firmware_loader: EFI firmware loader must handle pre-allocated buffer fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum fs/kernel_read_file: Split into separate source file fs/kernel_read_file: Remove redundant size argument fs/kernel_read_file: Switch buffer size arg to size_t fs/kernel_read_file: Add file_size output argument LSM: Introduce kernel_post_load_data() hook firmware_loader: Use security_post_load_data() module: Call security_kernel_post_load_data() LSM: Add "contents" flag to kernel_read_file hook fs/kernel_file_read: Add "offset" arg for partial reads Scott Branden (1): fs/kernel_read_file: Split into separate include file drivers/base/firmware_loader/fallback.c | 8 +- .../base/firmware_loader/fallback_platform.c | 12 +- drivers/base/firmware_loader/main.c | 13 +- fs/Makefile | 3 +- fs/exec.c | 132 +--- fs/kernel_read_file.c | 189 ++ include/linux/fs.h| 39 include/linux/ima.h | 19 +- include/linux/kernel_read_file.h | 55 + include/linux/lsm_hook_defs.h | 6 +- include/linux/lsm_hooks.h | 12 ++ include/linux/security.h | 19 +- kernel/kexec.c| 2 +- kernel/kexec_file.c | 18 +- kernel/module.c | 24 ++- security/integrity/digsig.c | 8 +- security/integrity/ima/ima_fs.c | 9 +- security/integrity/ima/ima_main.c | 58 -- security/integrity/ima/ima_policy.c | 1 + security/loadpin/loadpin.c| 17 +- security/security.c | 26 ++- security/selinux/hooks.c | 8 +- 22 files changed, 432 insertions(+), 246 deletions(-) create mode 100644 fs/kernel_read_file.c create mode 100644 include/linux/kernel_read_file.h ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 05/13] fs/kernel_read_file: Split into separate source file
On 2020-07-17 10:43 a.m., Kees Cook wrote:
These routines are used in places outside of exec(2), so in preparation
for refactoring them, move them into a separate source file,
fs/kernel_read_file.c.
Signed-off-by: Kees Cook
Acked-by: Scott Branden
---
fs/Makefile | 3 +-
fs/exec.c | 132
fs/kernel_read_file.c | 138 ++
3 files changed, 140 insertions(+), 133 deletions(-)
create mode 100644 fs/kernel_read_file.c
diff --git a/fs/Makefile b/fs/Makefile
index 2ce5112b02c8..a05fc247b2a7 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -13,7 +13,8 @@ obj-y := open.o read_write.o file_table.o super.o \
seq_file.o xattr.o libfs.o fs-writeback.o \
pnode.o splice.o sync.o utimes.o d_path.o \
stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
- fs_types.o fs_context.o fs_parser.o fsopen.o
+ fs_types.o fs_context.o fs_parser.o fsopen.o \
+ kernel_read_file.o
ifeq ($(CONFIG_BLOCK),y)
obj-y += buffer.o block_dev.o direct-io.o mpage.o
diff --git a/fs/exec.c b/fs/exec.c
index 07a7fe9ac5be..d619b79aab30 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -923,138 +923,6 @@ struct file *open_exec(const char *name)
}
EXPORT_SYMBOL(open_exec);
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
-loff_t max_size, enum kernel_read_file_id id)
-{
- loff_t i_size, pos;
- ssize_t bytes = 0;
- void *allocated = NULL;
- int ret;
-
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
- return -EINVAL;
-
- ret = deny_write_access(file);
- if (ret)
- return ret;
-
- ret = security_kernel_read_file(file, id);
- if (ret)
- goto out;
-
- i_size = i_size_read(file_inode(file));
- if (i_size <= 0) {
- ret = -EINVAL;
- goto out;
- }
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
- ret = -EFBIG;
- goto out;
- }
-
- if (!*buf)
- *buf = allocated = vmalloc(i_size);
- if (!*buf) {
- ret = -ENOMEM;
- goto out;
- }
-
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, );
- if (bytes < 0) {
- ret = bytes;
- goto out_free;
- }
-
- if (bytes == 0)
- break;
- }
-
- if (pos != i_size) {
- ret = -EIO;
- goto out_free;
- }
-
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
-
-out_free:
- if (ret < 0) {
- if (allocated) {
- vfree(*buf);
- *buf = NULL;
- }
- }
-
-out:
- allow_write_access(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file);
-
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
- loff_t max_size, enum kernel_read_file_id id)
-{
- struct file *file;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- file = filp_open(path, O_RDONLY, 0);
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
-
-int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
- enum kernel_read_file_id id)
-{
- struct file *file;
- struct path root;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- task_lock(_task);
- get_fs_root(init_task.fs, );
- task_unlock(_task);
-
- file = file_open_root(root.dentry, root.mnt, path, O_RDONLY, 0);
- path_put();
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-
-int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
-enum kernel_read_file_id id)
-{
- struct fd f = fdget(fd);
- int ret = -EBADF;
-
- if (!f.file)
- goto out;
-
- ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
- fdput(f);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
-
#if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
defined(CONFIG_BINFMT_ELF_FDPIC)
ssize_t read_code(struct file *file,
Re: [PATCH 03/13] fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum
On 2020-07-17 10:42 a.m., Kees Cook wrote:
The "FIRMWARE_EFI_EMBEDDED" enum is a "where", not a "what". It
should not be distinguished separately from just "FIRMWARE", as this
confuses the LSMs about what is being loaded. Additionally, there was
no actual validation of the firmware contents happening.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and
firmware_request_platform()")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
Acked-by: Scott Branden
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 2 +-
include/linux/fs.h | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index 685edb7dd05a..6958ab1a8059 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE_EFI_EMBEDDED);
+ rc = security_kernel_load_data(LOADING_FIRMWARE);
if (rc)
return rc;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 95fc775ed937..f50a35d54a61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,11 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
-/* This is a list of *what* is being read, not *how*. */
+/* This is a list of *what* is being read, not *how* nor *where*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
- id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
id(KEXEC_INITRAMFS, kexec-initramfs)\
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 02/13] fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum
On 2020-07-17 10:42 a.m., Kees Cook wrote:
FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
that are interested in filtering between types of things. The "how"
should be an internal detail made uninteresting to the LSMs.
Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated
buffer)")
Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware
(pre-allocated buffer)")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
Acked-by: Scott Branden
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/main.c | 5 ++---
fs/exec.c | 7 ---
include/linux/fs.h | 2 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
security/integrity/ima/ima_main.c | 6 ++
7 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index ca871b13524e..c2f57cedcd6f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
int i, len;
int rc = -ENOENT;
char *path;
- enum kernel_read_file_id id = READING_FIRMWARE;
size_t msize = INT_MAX;
void *buffer = NULL;
/* Already populated data member means we're loading into a buffer */
if (!decompress && fw_priv->data) {
buffer = fw_priv->data;
- id = READING_FIRMWARE_PREALLOC_BUFFER;
msize = fw_priv->allocated_size;
}
@@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, ,
- , msize, id);
+ , msize,
+ READING_FIRMWARE);
if (rc) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error
%d\n",
diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..2bf549757ce7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
{
loff_t i_size, pos;
ssize_t bytes = 0;
+ void *allocated = NULL;
int ret;
if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
@@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if (!*buf)
+ *buf = allocated = vmalloc(i_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
@@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
out_free:
if (ret < 0) {
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ if (allocated) {
vfree(*buf);
*buf = NULL;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f881a892ea7..95fc775ed937 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
+/* This is a list of *what* is being read, not *how*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
- id(FIRMWARE_PREALLOC_BUFFER, firmware) \
id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
diff --git a/kernel/module.c b/kernel/module.c
index 0c6573b98c36..26105148f4d2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3988,7 +3988,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user
*, uargs, int, flags)
{
struct load_info info = { };
loff_t size;
- void *hdr;
+ void *hdr = NULL;
int err;
err = may_init_module();
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index e9cbadade74b..ac02b7632353 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -169,7 +169,7 @@ int __init integrity_add_key(const unsigned int id, const
void *data,
int __init integrity_load_x509(const unsigned int id, const char *path)
{
- void *data;
+ void *data = NULL;
loff_t size;
int rc;
key_perm_t
Re: [PATCH 01/13] firmware_loader: EFI firmware loader must handle pre-allocated buffer
On 2020-07-17 10:42 a.m., Kees Cook wrote:
The EFI platform firmware fallback would clobber any pre-allocated
buffers. Instead, correctly refuse to reallocate when too small (as
already done in the sysfs fallback), or perform allocation normally
when needed.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firm
ware_request_platform()")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
Acked-by: Scott Branden
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index cdd2c9a9f38a..685edb7dd05a 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -25,7 +25,10 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (rc)
return rc; /* rc == -ENOENT when the fw was not found */
- fw_priv->data = vmalloc(size);
+ if (fw_priv->data && size > fw_priv->allocated_size)
+ return -ENOMEM;
+ if (!fw_priv->data)
+ fw_priv->data = vmalloc(size);
if (!fw_priv->data)
return -ENOMEM;
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
Hi Kees,
On 2020-07-17 10:43 a.m., Kees Cook wrote:
In preparation for refactoring kernel_read_file*(), remove the redundant
"size" argument which is not needed: it can be included in the return
I don't think the size argument is redundant though.
The existing kernel_read_file functions always read the whole file.
Now, what happens if the file is bigger than the buffer.
How does kernel_read_file know it read the whole file by looking at the
return value?
code, with callers adjusted. (VFS reads already cannot be larger than
INT_MAX.)
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 8
fs/kernel_read_file.c | 20 +---
include/linux/kernel_read_file.h| 8
kernel/kexec_file.c | 13 ++---
kernel/module.c | 7 +++
security/integrity/digsig.c | 5 +++--
security/integrity/ima/ima_fs.c | 5 +++--
7 files changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index d4a413ea48ce..ea419c7d3d34 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -462,7 +462,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
size_t in_size,
const void *in_buffer))
{
- loff_t size;
+ size_t size;
int i, len;
int rc = -ENOENT;
char *path;
@@ -494,10 +494,9 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, ,
- , msize,
+ rc = kernel_read_file_from_path_initns(path, , msize,
READING_FIRMWARE);
- if (rc) {
+ if (rc < 0) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error
%d\n",
path, rc);
@@ -506,6 +505,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
path);
continue;
}
+ size = rc;
dev_dbg(device, "Loading firmware from %s\n", path);
if (decompress) {
dev_dbg(device, "f/w decompressing %s\n",
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 54d972d4befc..dc28a8def597 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,7 +5,7 @@
#include
#include
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
+int kernel_read_file(struct file *file, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
@@ -29,7 +29,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
ret = -EINVAL;
goto out;
}
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
Should this be SSIZE_MAX?
ret = -EFBIG;
goto out;
}
@@ -59,8 +59,6 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
}
ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
out_free:
if (ret < 0) {
@@ -72,11 +70,11 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
out:
allow_write_access(file);
- return ret;
+ return ret == 0 ? pos : ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file);
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
struct file *file;
@@ -89,14 +87,14 @@ int kernel_read_file_from_path(const char *path, void
**buf, loff_t *size,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, max_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
+ loff_t max_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -115,13 +113,13 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
[PATCH 03/13] fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum
The "FIRMWARE_EFI_EMBEDDED" enum is a "where", not a "what". It
should not be distinguished separately from just "FIRMWARE", as this
confuses the LSMs about what is being loaded. Additionally, there was
no actual validation of the firmware contents happening.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and
firmware_request_platform()")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 2 +-
include/linux/fs.h | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index 685edb7dd05a..6958ab1a8059 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE_EFI_EMBEDDED);
+ rc = security_kernel_load_data(LOADING_FIRMWARE);
if (rc)
return rc;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 95fc775ed937..f50a35d54a61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,11 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
-/* This is a list of *what* is being read, not *how*. */
+/* This is a list of *what* is being read, not *how* nor *where*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
- id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
id(KEXEC_INITRAMFS, kexec-initramfs)\
--
2.25.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 08/13] fs/kernel_read_file: Add file_size output argument
In preparation for adding partial read support, add an optional output
argument to kernel_read_file*() that reports the file size so callers
can reason more easily about their reading progress.
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 1 +
fs/kernel_read_file.c | 19 +--
include/linux/kernel_read_file.h| 4
kernel/kexec_file.c | 4 ++--
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
7 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index ea419c7d3d34..3439a533927c 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -495,6 +495,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, , msize,
+ NULL,
READING_FIRMWARE);
if (rc < 0) {
if (rc != -ENOENT)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index e21a76001fff..2e29c38eb4df 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -14,6 +14,8 @@
* @buf_size will be ignored)
* @buf_size size of buf, if already allocated. If @buf not
* allocated, this is the largest size to allocate.
+ * @file_size if non-NULL, the full size of @file will be
+ * written here.
* @id the kernel_read_file_id identifying the type of
* file contents being read (for LSMs to examine)
*
@@ -22,7 +24,8 @@
*
*/
int kernel_read_file(struct file *file, void **buf,
-size_t buf_size, enum kernel_read_file_id id)
+size_t buf_size, size_t *file_size,
+enum kernel_read_file_id id)
{
loff_t i_size, pos;
ssize_t bytes = 0;
@@ -49,6 +52,8 @@ int kernel_read_file(struct file *file, void **buf,
ret = -EFBIG;
goto out;
}
+ if (file_size)
+ *file_size = i_size;
if (!*buf)
*buf = allocated = vmalloc(i_size);
@@ -91,7 +96,8 @@ int kernel_read_file(struct file *file, void **buf,
EXPORT_SYMBOL_GPL(kernel_read_file);
int kernel_read_file_from_path(const char *path, void **buf,
- size_t buf_size, enum kernel_read_file_id id)
+ size_t buf_size, size_t *file_size,
+ enum kernel_read_file_id id)
{
struct file *file;
int ret;
@@ -103,14 +109,14 @@ int kernel_read_file_from_path(const char *path, void
**buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, buf_size, id);
+ ret = kernel_read_file(file, buf, buf_size, file_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- size_t buf_size,
+ size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -129,13 +135,14 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, buf_size, id);
+ ret = kernel_read_file(file, buf, buf_size, file_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
+size_t *file_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -144,7 +151,7 @@ int kernel_read_file_from_fd(int fd, void **buf, size_t
buf_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, buf_size, id);
+ ret = kernel_read_file(f.file, buf, buf_size, file_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 910039e7593e..023293eaf948 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -37,15 +37,19 @@ static inline const char *kernel_read_file_id_str(enum
kernel_read_file_id id)
int kernel_read_file(struct file *file,
void **buf, size_t buf_size,
+size_t *file_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path(const char *path,
void **buf, size_t
[PATCH 13/13] fs/kernel_file_read: Add "offset" arg for partial reads
To perform partial reads, callers of kernel_read_file*() must have a
non-NULL file_size argument and a preallocated buffer. The new "offset"
argument can then be used to seek to specific locations in the file to
fill the buffer to, at most, "buf_size" per call.
Where possible, the LSM hooks can report whether a full file has been
read or not so that the contents can be reasoned about.
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 2 +-
fs/kernel_read_file.c | 78 -
include/linux/kernel_read_file.h| 8 +--
kernel/kexec_file.c | 4 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 3 +-
7 files changed, 65 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index 3439a533927c..fa540ca51961 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -494,7 +494,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, , msize,
+ rc = kernel_read_file_from_path_initns(path, 0, , msize,
NULL,
READING_FIRMWARE);
if (rc < 0) {
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index d73bc3fa710a..90d255fbdd9b 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -9,6 +9,7 @@
* kernel_read_file() - read file contents into a kernel buffer
*
* @file file to read from
+ * @offset where to start reading from (see below).
* @bufpointer to a "void *" buffer for reading into (if
* *@buf is NULL, a buffer will be allocated, and
* @buf_size will be ignored)
@@ -19,19 +20,31 @@
* @id the kernel_read_file_id identifying the type of
* file contents being read (for LSMs to examine)
*
+ * @offset must be 0 unless both @buf and @file_size are non-NULL
+ * (i.e. the caller must be expecting to read partial file contents
+ * via an already-allocated @buf, in at most @buf_size chunks, and
+ * will be able to determine when the entire file was read by
+ * checking @file_size). This isn't a recommended way to read a
+ * file, though, since it is possible that the contents might
+ * change between calls to kernel_read_file().
+ *
* Returns number of bytes read (no single read will be bigger
* than INT_MAX), or negative on error.
*
*/
-int kernel_read_file(struct file *file, void **buf,
+int kernel_read_file(struct file *file, loff_t offset, void **buf,
size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
loff_t i_size, pos;
- ssize_t bytes = 0;
+ size_t copied;
void *allocated = NULL;
+ bool whole_file;
int ret;
+ if (offset != 0 && (!*buf || !file_size))
+ return -EINVAL;
+
if (!S_ISREG(file_inode(file)->i_mode))
return -EINVAL;
@@ -39,19 +52,27 @@ int kernel_read_file(struct file *file, void **buf,
if (ret)
return ret;
- ret = security_kernel_read_file(file, id, true);
- if (ret)
- goto out;
-
i_size = i_size_read(file_inode(file));
if (i_size <= 0) {
ret = -EINVAL;
goto out;
}
- if (i_size > INT_MAX || i_size > buf_size) {
+ /* The file is too big for sane activities. */
+ if (i_size > INT_MAX) {
+ ret = -EFBIG;
+ goto out;
+ }
+ /* The entire file cannot be read in one buffer. */
+ if (!file_size && offset == 0 && i_size > buf_size) {
ret = -EFBIG;
goto out;
}
+
+ whole_file = (offset == 0 && i_size <= buf_size);
+ ret = security_kernel_read_file(file, id, whole_file);
+ if (ret)
+ goto out;
+
if (file_size)
*file_size = i_size;
@@ -62,9 +83,14 @@ int kernel_read_file(struct file *file, void **buf,
goto out;
}
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, );
+ pos = offset;
+ copied = 0;
+ while (copied < buf_size) {
+ ssize_t bytes;
+ size_t wanted = min_t(size_t, buf_size - copied,
+ i_size - pos);
+
+ bytes = kernel_read(file, *buf + copied, wanted, );
if (bytes < 0) {
ret = bytes;
goto out_free;
@@ -72,14 +98,17 @@ int kernel_read_file(struct
[PATCH 09/13] LSM: Introduce kernel_post_load_data() hook
There are a few places in the kernel where LSMs would like to have
visibility into the contents of a kernel buffer that has been loaded or
read. While security_kernel_post_read_file() (which includes the
buffer) exists as a pairing for security_kernel_read_file(), no such
hook exists to pair with security_kernel_load_data().
Earlier proposals for just using security_kernel_post_read_file() with a
NULL file argument were rejected (i.e. "file" should always be valid for
the security_..._file hooks, but it appears at least one case was
left in the kernel during earlier refactoring. (This will be fixed in
a subsequent patch.)
Since not all cases of security_kernel_load_data() can have a single
contiguous buffer made available to the LSM hook (e.g. kexec image
segments are separately loaded), there needs to be a way for the LSM to
reason about its expectations of the hook coverage. In order to handle
this, add a "contents" argument to the "kernel_load_data" hook that
indicates if the newly added "kernel_post_load_data" hook will be called
with the full contents once loaded. That way, LSMs requiring full contents
can choose to unilaterally reject "kernel_load_data" with contents=false
(which is effectively the existing hook coverage), but when contents=true
they can allow it and later evaluate the "kernel_post_load_data" hook
once the buffer is loaded.
With this change, LSMs can gain coverage over non-file-backed data loads
(e.g. init_module(2) and firmware userspace helper), which will happen
in subsequent patches.
Additionally prepare IMA to start processing these cases.
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/fallback.c | 2 +-
.../base/firmware_loader/fallback_platform.c | 2 +-
include/linux/ima.h | 12 +--
include/linux/lsm_hook_defs.h | 4 +++-
include/linux/lsm_hooks.h | 9
include/linux/security.h | 12 +--
kernel/kexec.c| 2 +-
kernel/module.c | 2 +-
security/integrity/ima/ima_main.c | 21 ++-
security/loadpin/loadpin.c| 2 +-
security/security.c | 18 +---
security/selinux/hooks.c | 2 +-
12 files changed, 73 insertions(+), 15 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 5327bfc6ba71..a196aacce22c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -613,7 +613,7 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
return false;
/* Also permit LSMs and IMA to fail firmware sysfs fallback */
- ret = security_kernel_load_data(LOADING_FIRMWARE);
+ ret = security_kernel_load_data(LOADING_FIRMWARE, false);
if (ret < 0)
return false;
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index 6958ab1a8059..a12c79d47efc 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE);
+ rc = security_kernel_load_data(LOADING_FIRMWARE, false);
if (rc)
return rc;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 148636bfcc8f..502e36ad7804 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,9 @@ extern void ima_post_create_tmpfile(struct inode *inode);
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);
extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
-extern int ima_load_data(enum kernel_load_data_id id);
+extern int ima_load_data(enum kernel_load_data_id id, bool contents);
+extern int ima_post_load_data(char *buf, loff_t size,
+ enum kernel_load_data_id id);
extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
@@ -78,7 +80,13 @@ static inline int ima_file_mprotect(struct vm_area_struct
*vma,
return 0;
}
-static inline int ima_load_data(enum kernel_load_data_id id)
+static inline int ima_load_data(enum kernel_load_data_id id, bool contents)
+{
+ return 0;
+}
+
+static inline int ima_post_load_data(char *buf, loff_t size,
+enum kernel_load_data_id id)
{
return 0;
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 6791813cd439..aaa2916bbae7 100644
---
[PATCH 12/13] LSM: Add "contents" flag to kernel_read_file hook
As with the kernel_load_data LSM hook, add a "contents" flag to the
kernel_read_file LSM hook that indicates whether the LSM can expect
a matching call to the kernel_post_read_file LSM hook with the full
contents of the file. With the coming addition of partial file read
support for kernel_read_file*() API, the LSM will no longer be able
to always see the entire contents of a file during the read calls.
For cases where the LSM must read examine the complete file contents,
it will need to do so on its own every time the kernel_read_file
hook is called with contents=false (or reject such cases). Adjust all
existing LSMs to retain existing behavior.
Signed-off-by: Kees Cook
---
fs/kernel_read_file.c | 2 +-
include/linux/ima.h | 6 --
include/linux/lsm_hook_defs.h | 2 +-
include/linux/lsm_hooks.h | 3 +++
include/linux/security.h | 6 --
security/integrity/ima/ima_main.c | 10 +-
security/loadpin/loadpin.c| 14 --
security/security.c | 7 ---
security/selinux/hooks.c | 5 +++--
9 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 2e29c38eb4df..d73bc3fa710a 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -39,7 +39,7 @@ int kernel_read_file(struct file *file, void **buf,
if (ret)
return ret;
- ret = security_kernel_read_file(file, id);
+ ret = security_kernel_read_file(file, id, true);
if (ret)
goto out;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 502e36ad7804..259023039dc9 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -23,7 +23,8 @@ extern int ima_file_mprotect(struct vm_area_struct *vma,
unsigned long prot);
extern int ima_load_data(enum kernel_load_data_id id, bool contents);
extern int ima_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id);
-extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
+extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
+bool contents);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
@@ -91,7 +92,8 @@ static inline int ima_post_load_data(char *buf, loff_t size,
return 0;
}
-static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
+static inline int ima_read_file(struct file *file, enum kernel_read_file_id id,
+ bool contents)
{
return 0;
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index aaa2916bbae7..c2ded57c5d9b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -188,7 +188,7 @@ LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id
id, bool contents)
LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
enum kernel_read_file_id id)
LSM_HOOK(int, 0, kernel_read_file, struct file *file,
-enum kernel_read_file_id id)
+enum kernel_read_file_id id, bool contents)
LSM_HOOK(int, 0, kernel_post_read_file, struct file *file, char *buf,
loff_t size, enum kernel_read_file_id id)
LSM_HOOK(int, 0, task_fix_setuid, struct cred *new, const struct cred *old,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 812d626195fc..b66433b5aa15 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -650,6 +650,7 @@
* @file contains the file structure pointing to the file being read
* by the kernel.
* @id kernel read file identifier
+ * @contents if a subsequent @kernel_post_read_file will be called.
* Return 0 if permission is granted.
* @kernel_post_read_file:
* Read a file specified by userspace.
@@ -658,6 +659,8 @@
* @buf pointer to buffer containing the file contents.
* @size length of the file contents.
* @id kernel read file identifier
+ * This must be paired with a prior @kernel_read_file call that had
+ * @contents set to true.
* Return 0 if permission is granted.
* @task_fix_setuid:
* Update the module's state after setting one or more of the user
diff --git a/include/linux/security.h b/include/linux/security.h
index e748974c707b..a5d66b89cd6c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -390,7 +390,8 @@ int security_kernel_module_request(char *kmod_name);
int security_kernel_load_data(enum kernel_load_data_id id, bool contents);
int security_kernel_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id);
-int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
+int security_kernel_read_file(struct file *file, enum kernel_read_file_id id,
+
[PATCH 04/13] fs/kernel_read_file: Split into separate include file
From: Scott Branden
Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h
include file. That header gets pulled in just about everywhere
and doesn't really need functions not related to the general fs interface.
Suggested-by: Christoph Hellwig
Signed-off-by: Scott Branden
Reviewed-by: Christoph Hellwig
Acked-by: Greg Kroah-Hartman
Link:
https://lore.kernel.org/r/20200706232309.12010-2-scott.bran...@broadcom.com
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 1 +
fs/exec.c | 1 +
include/linux/fs.h | 38 -
include/linux/ima.h | 1 +
include/linux/kernel_read_file.h| 51 +
include/linux/security.h| 1 +
kernel/kexec_file.c | 1 +
kernel/module.c | 1 +
security/integrity/digsig.c | 1 +
security/integrity/ima/ima_fs.c | 1 +
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_policy.c | 1 +
security/loadpin/loadpin.c | 1 +
security/security.c | 1 +
security/selinux/hooks.c| 1 +
15 files changed, 64 insertions(+), 38 deletions(-)
create mode 100644 include/linux/kernel_read_file.h
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index c2f57cedcd6f..d4a413ea48ce 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -12,6 +12,7 @@
#include
#include
+#include
#include
#include
#include
diff --git a/fs/exec.c b/fs/exec.c
index 2bf549757ce7..07a7fe9ac5be 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -23,6 +23,7 @@
* formats.
*/
+#include
#include
#include
#include
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f50a35d54a61..11dd6cc7de58 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,44 +2993,6 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
-/* This is a list of *what* is being read, not *how* nor *where*. */
-#define __kernel_read_file_id(id) \
- id(UNKNOWN, unknown)\
- id(FIRMWARE, firmware) \
- id(MODULE, kernel-module) \
- id(KEXEC_IMAGE, kexec-image)\
- id(KEXEC_INITRAMFS, kexec-initramfs)\
- id(POLICY, security-policy) \
- id(X509_CERTIFICATE, x509-certificate) \
- id(MAX_ID, )
-
-#define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
-#define __fid_stringify(dummy, str) #str,
-
-enum kernel_read_file_id {
- __kernel_read_file_id(__fid_enumify)
-};
-
-static const char * const kernel_read_file_str[] = {
- __kernel_read_file_id(__fid_stringify)
-};
-
-static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
-{
- if ((unsigned)id >= READING_MAX_ID)
- return kernel_read_file_str[READING_UNKNOWN];
-
- return kernel_read_file_str[id];
-}
-
-extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
-extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
-extern int kernel_read_file_from_path_initns(const char *, void **, loff_t *,
loff_t,
-enum kernel_read_file_id);
-extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
extern ssize_t kernel_read(struct file *, void *, size_t, loff_t *);
extern ssize_t kernel_write(struct file *, const void *, size_t, loff_t *);
extern ssize_t __kernel_write(struct file *, const void *, size_t, loff_t *);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..148636bfcc8f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -7,6 +7,7 @@
#ifndef _LINUX_IMA_H
#define _LINUX_IMA_H
+#include
#include
#include
#include
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
new file mode 100644
index ..78cf3d7dc835
--- /dev/null
+++ b/include/linux/kernel_read_file.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_KERNEL_READ_FILE_H
+#define _LINUX_KERNEL_READ_FILE_H
+
+#include
+#include
+
+/* This is a list of *what* is being read, not *how* nor *where*. */
+#define __kernel_read_file_id(id) \
+ id(UNKNOWN, unknown)\
+ id(FIRMWARE, firmware) \
+ id(MODULE, kernel-module) \
+ id(KEXEC_IMAGE, kexec-image)\
+ id(KEXEC_INITRAMFS, kexec-initramfs)\
+ id(POLICY, security-policy) \
+ id(X509_CERTIFICATE, x509-certificate) \
+ id(MAX_ID, )
+
+#define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
+#define __fid_stringify(dummy, str) #str,
+
+enum
[PATCH 07/13] fs/kernel_read_file: Switch buffer size arg to size_t
In preparation for further refactoring of kernel_read_file*(), rename
the "max_size" argument to the more accurate "buf_size", and correct
its type to size_t. Add kerndoc to explain the specifics of how the
arguments will be used. Note that with buf_size now size_t, it can no
longer be negative (and was never called with a negative value). Adjust
callers to use it as a "maximum size" when *buf is NULL.
Signed-off-by: Kees Cook
---
fs/kernel_read_file.c| 34 +++-
include/linux/kernel_read_file.h | 8
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
4 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index dc28a8def597..e21a76001fff 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,15 +5,31 @@
#include
#include
+/**
+ * kernel_read_file() - read file contents into a kernel buffer
+ *
+ * @file file to read from
+ * @bufpointer to a "void *" buffer for reading into (if
+ * *@buf is NULL, a buffer will be allocated, and
+ * @buf_size will be ignored)
+ * @buf_size size of buf, if already allocated. If @buf not
+ * allocated, this is the largest size to allocate.
+ * @id the kernel_read_file_id identifying the type of
+ * file contents being read (for LSMs to examine)
+ *
+ * Returns number of bytes read (no single read will be bigger
+ * than INT_MAX), or negative on error.
+ *
+ */
int kernel_read_file(struct file *file, void **buf,
-loff_t max_size, enum kernel_read_file_id id)
+size_t buf_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
ssize_t bytes = 0;
void *allocated = NULL;
int ret;
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
+ if (!S_ISREG(file_inode(file)->i_mode))
return -EINVAL;
ret = deny_write_access(file);
@@ -29,7 +45,7 @@ int kernel_read_file(struct file *file, void **buf,
ret = -EINVAL;
goto out;
}
- if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || i_size > buf_size) {
ret = -EFBIG;
goto out;
}
@@ -75,7 +91,7 @@ int kernel_read_file(struct file *file, void **buf,
EXPORT_SYMBOL_GPL(kernel_read_file);
int kernel_read_file_from_path(const char *path, void **buf,
- loff_t max_size, enum kernel_read_file_id id)
+ size_t buf_size, enum kernel_read_file_id id)
{
struct file *file;
int ret;
@@ -87,14 +103,14 @@ int kernel_read_file_from_path(const char *path, void
**buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, max_size, id);
+ ret = kernel_read_file(file, buf, buf_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t max_size,
+ size_t buf_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -113,13 +129,13 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, max_size, id);
+ ret = kernel_read_file(file, buf, buf_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-int kernel_read_file_from_fd(int fd, void **buf, loff_t max_size,
+int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -128,7 +144,7 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t
max_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, max_size, id);
+ ret = kernel_read_file(f.file, buf, buf_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 0ca0bdbed1bd..910039e7593e 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -36,16 +36,16 @@ static inline const char *kernel_read_file_id_str(enum
kernel_read_file_id id)
}
int kernel_read_file(struct file *file,
-void **buf, loff_t max_size,
+void **buf, size_t buf_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path(const char *path,
- void **buf, loff_t max_size,
+ void **buf, size_t buf_size,
enum kernel_read_file_id id);
int
[PATCH 05/13] fs/kernel_read_file: Split into separate source file
These routines are used in places outside of exec(2), so in preparation
for refactoring them, move them into a separate source file,
fs/kernel_read_file.c.
Signed-off-by: Kees Cook
---
fs/Makefile | 3 +-
fs/exec.c | 132
fs/kernel_read_file.c | 138 ++
3 files changed, 140 insertions(+), 133 deletions(-)
create mode 100644 fs/kernel_read_file.c
diff --git a/fs/Makefile b/fs/Makefile
index 2ce5112b02c8..a05fc247b2a7 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -13,7 +13,8 @@ obj-y := open.o read_write.o file_table.o super.o \
seq_file.o xattr.o libfs.o fs-writeback.o \
pnode.o splice.o sync.o utimes.o d_path.o \
stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
- fs_types.o fs_context.o fs_parser.o fsopen.o
+ fs_types.o fs_context.o fs_parser.o fsopen.o \
+ kernel_read_file.o
ifeq ($(CONFIG_BLOCK),y)
obj-y += buffer.o block_dev.o direct-io.o mpage.o
diff --git a/fs/exec.c b/fs/exec.c
index 07a7fe9ac5be..d619b79aab30 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -923,138 +923,6 @@ struct file *open_exec(const char *name)
}
EXPORT_SYMBOL(open_exec);
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
-loff_t max_size, enum kernel_read_file_id id)
-{
- loff_t i_size, pos;
- ssize_t bytes = 0;
- void *allocated = NULL;
- int ret;
-
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
- return -EINVAL;
-
- ret = deny_write_access(file);
- if (ret)
- return ret;
-
- ret = security_kernel_read_file(file, id);
- if (ret)
- goto out;
-
- i_size = i_size_read(file_inode(file));
- if (i_size <= 0) {
- ret = -EINVAL;
- goto out;
- }
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
- ret = -EFBIG;
- goto out;
- }
-
- if (!*buf)
- *buf = allocated = vmalloc(i_size);
- if (!*buf) {
- ret = -ENOMEM;
- goto out;
- }
-
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, );
- if (bytes < 0) {
- ret = bytes;
- goto out_free;
- }
-
- if (bytes == 0)
- break;
- }
-
- if (pos != i_size) {
- ret = -EIO;
- goto out_free;
- }
-
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
-
-out_free:
- if (ret < 0) {
- if (allocated) {
- vfree(*buf);
- *buf = NULL;
- }
- }
-
-out:
- allow_write_access(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file);
-
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
- loff_t max_size, enum kernel_read_file_id id)
-{
- struct file *file;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- file = filp_open(path, O_RDONLY, 0);
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
-
-int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
- enum kernel_read_file_id id)
-{
- struct file *file;
- struct path root;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- task_lock(_task);
- get_fs_root(init_task.fs, );
- task_unlock(_task);
-
- file = file_open_root(root.dentry, root.mnt, path, O_RDONLY, 0);
- path_put();
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-
-int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
-enum kernel_read_file_id id)
-{
- struct fd f = fdget(fd);
- int ret = -EBADF;
-
- if (!f.file)
- goto out;
-
- ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
- fdput(f);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
-
#if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
defined(CONFIG_BINFMT_ELF_FDPIC)
ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t
len)
diff --git a/fs/kernel_read_file.c
[PATCH 10/13] firmware_loader: Use security_post_load_data()
Now that security_post_load_data() is wired up, use it instead
of the NULL file argument style of security_post_read_file(),
and update the security_kernel_load_data() call to indicate that a
security_kernel_post_load_data() call is expected.
Wire up the IMA check to match earlier logic. Perhaps a generalized
change to ima_post_load_data() might look something like this:
return process_buffer_measurement(buf, size,
kernel_load_data_id_str(load_id),
read_idmap[load_id] ?: FILE_CHECK,
0, NULL);
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/fallback.c | 8
.../base/firmware_loader/fallback_platform.c | 7 ++-
security/integrity/ima/ima_main.c | 20 +--
3 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index a196aacce22c..7cfdfdcb819c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -272,9 +272,9 @@ static ssize_t firmware_loading_store(struct device *dev,
dev_err(dev, "%s: map pages failed\n",
__func__);
else
- rc = security_kernel_post_read_file(NULL,
- fw_priv->data, fw_priv->size,
- READING_FIRMWARE);
+ rc =
security_kernel_post_load_data(fw_priv->data,
+ fw_priv->size,
+ LOADING_FIRMWARE);
/*
* Same logic as fw_load_abort, only the DONE bit
@@ -613,7 +613,7 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
return false;
/* Also permit LSMs and IMA to fail firmware sysfs fallback */
- ret = security_kernel_load_data(LOADING_FIRMWARE, false);
+ ret = security_kernel_load_data(LOADING_FIRMWARE, true);
if (ret < 0)
return false;
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index a12c79d47efc..4d1157af0e86 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE, false);
+ rc = security_kernel_load_data(LOADING_FIRMWARE, true);
if (rc)
return rc;
@@ -27,6 +27,11 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (fw_priv->data && size > fw_priv->allocated_size)
return -ENOMEM;
+
+ rc = security_kernel_post_load_data((u8 *)data, size, LOADING_FIRMWARE);
+ if (rc)
+ return rc;
+
if (!fw_priv->data)
fw_priv->data = vmalloc(size);
if (!fw_priv->data)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 85000dc8595c..1a7bc4c7437d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -648,15 +648,6 @@ int ima_post_read_file(struct file *file, void *buf,
loff_t size,
enum ima_hooks func;
u32 secid;
- if (!file && read_id == READING_FIRMWARE) {
- if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
- (ima_appraise & IMA_APPRAISE_ENFORCE)) {
- pr_err("Prevent firmware loading_store.\n");
- return -EACCES; /* INTEGRITY_UNKNOWN */
- }
- return 0;
- }
-
/* permit signed certs */
if (!file && read_id == READING_X509_CERTIFICATE)
return 0;
@@ -706,7 +697,7 @@ int ima_load_data(enum kernel_load_data_id id, bool
contents)
}
break;
case LOADING_FIRMWARE:
- if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE)) {
+ if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE) &&
!contents) {
pr_err("Prevent firmware sysfs fallback loading.\n");
return -EACCES; /* INTEGRITY_UNKNOWN */
}
@@ -739,6 +730,15 @@ int ima_load_data(enum kernel_load_data_id id, bool
contents)
*/
int ima_post_load_data(char *buf, loff_t size, enum kernel_load_data_id
load_id)
{
+ if (load_id == LOADING_FIRMWARE) {
+ if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("Prevent firmware loading_store.\n");
+
[PATCH 11/13] module: Call security_kernel_post_load_data()
Now that there is an API for checking loaded contents for modules
loaded without a file, call into the LSM hooks.
Cc: Jessica Yu
Signed-off-by: Kees Cook
---
kernel/module.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index d56cb34d9a2f..90a4788dff9d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2967,7 +2967,7 @@ static int copy_module_from_user(const void __user *umod,
unsigned long len,
if (info->len < sizeof(*(info->hdr)))
return -ENOEXEC;
- err = security_kernel_load_data(LOADING_MODULE, false);
+ err = security_kernel_load_data(LOADING_MODULE, true);
if (err)
return err;
@@ -2977,11 +2977,17 @@ static int copy_module_from_user(const void __user
*umod, unsigned long len,
return -ENOMEM;
if (copy_chunked_from_user(info->hdr, umod, info->len) != 0) {
- vfree(info->hdr);
- return -EFAULT;
+ err = -EFAULT;
+ goto out;
}
- return 0;
+ err = security_kernel_post_load_data((char *)info->hdr, info->len,
+LOADING_MODULE);
+out:
+ if (err)
+ vfree(info->hdr);
+
+ return err;
}
static void free_copy(struct load_info *info)
--
2.25.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 02/13] fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum
FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
that are interested in filtering between types of things. The "how"
should be an internal detail made uninteresting to the LSMs.
Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures
(pre-allocated buffer)")
Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware
(pre-allocated buffer)")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/main.c | 5 ++---
fs/exec.c | 7 ---
include/linux/fs.h | 2 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
security/integrity/ima/ima_main.c | 6 ++
7 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index ca871b13524e..c2f57cedcd6f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
int i, len;
int rc = -ENOENT;
char *path;
- enum kernel_read_file_id id = READING_FIRMWARE;
size_t msize = INT_MAX;
void *buffer = NULL;
/* Already populated data member means we're loading into a buffer */
if (!decompress && fw_priv->data) {
buffer = fw_priv->data;
- id = READING_FIRMWARE_PREALLOC_BUFFER;
msize = fw_priv->allocated_size;
}
@@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, ,
- , msize, id);
+ , msize,
+ READING_FIRMWARE);
if (rc) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error
%d\n",
diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..2bf549757ce7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
{
loff_t i_size, pos;
ssize_t bytes = 0;
+ void *allocated = NULL;
int ret;
if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
@@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if (!*buf)
+ *buf = allocated = vmalloc(i_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
@@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
out_free:
if (ret < 0) {
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ if (allocated) {
vfree(*buf);
*buf = NULL;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f881a892ea7..95fc775ed937 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
+/* This is a list of *what* is being read, not *how*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
- id(FIRMWARE_PREALLOC_BUFFER, firmware) \
id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
diff --git a/kernel/module.c b/kernel/module.c
index 0c6573b98c36..26105148f4d2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3988,7 +3988,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user
*, uargs, int, flags)
{
struct load_info info = { };
loff_t size;
- void *hdr;
+ void *hdr = NULL;
int err;
err = may_init_module();
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index e9cbadade74b..ac02b7632353 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -169,7 +169,7 @@ int __init integrity_add_key(const unsigned int id, const
void *data,
int __init integrity_load_x509(const unsigned int id, const char *path)
{
- void *data;
+ void *data = NULL;
loff_t size;
int rc;
key_perm_t perm;
diff --git a/security/integrity/ima/ima_fs.c
[PATCH 01/13] firmware_loader: EFI firmware loader must handle pre-allocated buffer
The EFI platform firmware fallback would clobber any pre-allocated
buffers. Instead, correctly refuse to reallocate when too small (as
already done in the sysfs fallback), or perform allocation normally
when needed.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firm
ware_request_platform()")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index cdd2c9a9f38a..685edb7dd05a 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -25,7 +25,10 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (rc)
return rc; /* rc == -ENOENT when the fw was not found */
- fw_priv->data = vmalloc(size);
+ if (fw_priv->data && size > fw_priv->allocated_size)
+ return -ENOMEM;
+ if (!fw_priv->data)
+ fw_priv->data = vmalloc(size);
if (!fw_priv->data)
return -ENOMEM;
--
2.25.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
In preparation for refactoring kernel_read_file*(), remove the redundant
"size" argument which is not needed: it can be included in the return
code, with callers adjusted. (VFS reads already cannot be larger than
INT_MAX.)
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 8
fs/kernel_read_file.c | 20 +---
include/linux/kernel_read_file.h| 8
kernel/kexec_file.c | 13 ++---
kernel/module.c | 7 +++
security/integrity/digsig.c | 5 +++--
security/integrity/ima/ima_fs.c | 5 +++--
7 files changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index d4a413ea48ce..ea419c7d3d34 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -462,7 +462,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
size_t in_size,
const void *in_buffer))
{
- loff_t size;
+ size_t size;
int i, len;
int rc = -ENOENT;
char *path;
@@ -494,10 +494,9 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, ,
- , msize,
+ rc = kernel_read_file_from_path_initns(path, , msize,
READING_FIRMWARE);
- if (rc) {
+ if (rc < 0) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error
%d\n",
path, rc);
@@ -506,6 +505,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
path);
continue;
}
+ size = rc;
dev_dbg(device, "Loading firmware from %s\n", path);
if (decompress) {
dev_dbg(device, "f/w decompressing %s\n",
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 54d972d4befc..dc28a8def597 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,7 +5,7 @@
#include
#include
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
+int kernel_read_file(struct file *file, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
@@ -29,7 +29,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
ret = -EINVAL;
goto out;
}
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
ret = -EFBIG;
goto out;
}
@@ -59,8 +59,6 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
}
ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
out_free:
if (ret < 0) {
@@ -72,11 +70,11 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
out:
allow_write_access(file);
- return ret;
+ return ret == 0 ? pos : ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file);
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
struct file *file;
@@ -89,14 +87,14 @@ int kernel_read_file_from_path(const char *path, void
**buf, loff_t *size,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, max_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
+ loff_t max_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -115,13 +113,13 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, max_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
+int kernel_read_file_from_fd(int fd, void
[PATCH 00/13] Introduce partial kernel_read_file() support
Hi, Here's my attempt at clearing the path to partial read support in kernel_read_file(), which fixes a number of issues along the way. I'm still fighting with the firmware test suite (it doesn't seem to pass for me even in stock v5.7... ?) But I don't want to block Scott's work[1] any this week, so here's the series as it is currently. The primary difference to Scott's approach is to avoid adding a new set of functions and just adapt the existing APIs to deal with "offset". Also, the fixes for the enum are first in the series so they can be backported without the header file relocation. I'll keep poking at the firmware tests... -Kees [1] https://lore.kernel.org/lkml/202007161415.10D015477@keescook/ Kees Cook (12): firmware_loader: EFI firmware loader must handle pre-allocated buffer fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum fs/kernel_read_file: Split into separate source file fs/kernel_read_file: Remove redundant size argument fs/kernel_read_file: Switch buffer size arg to size_t fs/kernel_read_file: Add file_size output argument LSM: Introduce kernel_post_load_data() hook firmware_loader: Use security_post_load_data() module: Call security_kernel_post_load_data() LSM: Add "contents" flag to kernel_read_file hook fs/kernel_file_read: Add "offset" arg for partial reads Scott Branden (1): fs/kernel_read_file: Split into separate include file drivers/base/firmware_loader/fallback.c | 8 +- .../base/firmware_loader/fallback_platform.c | 12 +- drivers/base/firmware_loader/main.c | 13 +- fs/Makefile | 3 +- fs/exec.c | 132 +--- fs/kernel_read_file.c | 189 ++ include/linux/fs.h| 39 include/linux/ima.h | 19 +- include/linux/kernel_read_file.h | 55 + include/linux/lsm_hook_defs.h | 6 +- include/linux/lsm_hooks.h | 12 ++ include/linux/security.h | 19 +- kernel/kexec.c| 2 +- kernel/kexec_file.c | 18 +- kernel/module.c | 24 ++- security/integrity/digsig.c | 8 +- security/integrity/ima/ima_fs.c | 9 +- security/integrity/ima/ima_main.c | 58 -- security/integrity/ima/ima_policy.c | 1 + security/loadpin/loadpin.c| 17 +- security/security.c | 26 ++- security/selinux/hooks.c | 8 +- 22 files changed, 432 insertions(+), 246 deletions(-) create mode 100644 fs/kernel_read_file.c create mode 100644 include/linux/kernel_read_file.h -- 2.25.1 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
