[PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader
This commit converts the late loader in the AMD microcode update driver to
use newly introduced microcode container data checking functions as the
previous commit did for the early loader.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
arch/x86/kernel/cpu/microcode/amd.c | 87 +
1 file changed, 40 insertions(+), 47 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 94fcd702a67a..b429d3f554b9 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -677,28 +677,24 @@ static enum ucode_state apply_microcode_amd(int cpu)
return UCODE_UPDATED;
}
-static int install_equiv_cpu_table(const u8 *buf)
+static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
{
- unsigned int *ibuf = (unsigned int *)buf;
- unsigned int type = ibuf[1];
- unsigned int size = ibuf[2];
+ const u32 *hdr = (const u32 *)buf;
+ u32 equiv_tbl_len;
- if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
- pr_err("empty section/"
- "invalid type field in container file section header\n");
- return -EINVAL;
- }
+ if (!verify_equivalence_table(buf, buf_size, false))
+ return 0;
- equiv_cpu_table = vmalloc(size);
+ equiv_tbl_len = hdr[2];
+ equiv_cpu_table = vmalloc(equiv_tbl_len);
if (!equiv_cpu_table) {
pr_err("failed to allocate equivalent CPU table\n");
- return -ENOMEM;
+ return 0;
}
- memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
+ memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
- /* add header length */
- return size + CONTAINER_HDR_SZ;
+ return equiv_tbl_len;
}
static void free_equiv_cpu_table(void)
@@ -715,20 +711,26 @@ static void cleanup(void)
/*
* We return the current size even if some of the checks failed so that
- * we can skip over the next patch. If we return a negative value, we
- * signal a grave error like a memory allocation has failed and the
- * driver cannot continue functioning normally. In such cases, we tear
- * down everything we've used up so far and exit.
+ * we can skip over the next patch. If we return zero, we signal a
+ * grave error like a memory allocation has failed and the driver cannot
+ * continue functioning normally. In such cases, we tear down everything
+ * we've used up so far and exit.
*/
-static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
+static unsigned int verify_and_add_patch(u8 family, u8 *fw,
+unsigned int leftover)
{
+ u32 *hdr = (u32 *)fw;
struct microcode_header_amd *mc_hdr;
struct ucode_patch *patch;
- unsigned int patch_size, crnt_size, ret;
+ u32 patch_size;
+ unsigned int crnt_size;
u32 proc_fam;
u16 proc_id;
- patch_size = *(u32 *)(fw + 4);
+ if (!verify_patch_section(fw, leftover, false))
+ return leftover;
+
+ patch_size = hdr[1];
crnt_size = patch_size + SECTION_HDR_SIZE;
mc_hdr = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
proc_id = mc_hdr->processor_rev_id;
@@ -750,28 +752,20 @@ static int verify_and_add_patch(u8 family, u8 *fw,
unsigned int leftover)
return crnt_size;
}
- /*
-* The section header length is not included in this indicated size
-* but is present in the leftover file length so we need to subtract
-* it before passing this value to the function below.
-*/
- ret = verify_patch_size(family, patch_size, leftover -
SECTION_HDR_SIZE);
- if (!ret) {
- pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
+ if (!verify_patch(family, fw, leftover, false))
return crnt_size;
- }
patch = kzalloc(sizeof(*patch), GFP_KERNEL);
if (!patch) {
pr_err("Patch allocation failure.\n");
- return -EINVAL;
+ return 0;
}
patch->data = kmemdup(fw + SECTION_HDR_SIZE, patch_size, GFP_KERNEL);
if (!patch->data) {
pr_err("Patch data allocation failure.\n");
kfree(patch);
- return -EINVAL;
+ return 0;
}
INIT_LIST_HEAD(>plist);
@@ -793,26 +787,27 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
enum ucode_state ret = UCODE_ERROR;
unsigned int leftover;
u8 *fw = (u8 *)data;
- int crnt_size = 0;
- int offset;
+ unsigned int offset;
- offset = install_equiv_cpu_table(data);
- if (offset < 0) {
+ offset
[PATCH v5 5/6] x86/microcode/AMD: Add a reminder about PATCH_MAX_SIZE macro
The PATCH_MAX_SIZE macro should contain the maximum of all family patch
sizes.
Since these sizes are defined in an other place than this macro, let's add
a reminder to them so people will remember to verify PATCH_MAX_SIZE
correctness when modifying a family patch size or adding a new family.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
arch/x86/include/asm/microcode_amd.h | 1 +
arch/x86/kernel/cpu/microcode/amd.c | 4
2 files changed, 5 insertions(+)
diff --git a/arch/x86/include/asm/microcode_amd.h
b/arch/x86/include/asm/microcode_amd.h
index 209492849566..8ea477fbc65f 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -41,6 +41,7 @@ struct microcode_amd {
unsigned intmpb[0];
};
+/* Maximum patch size of all supported families */
#define PATCH_MAX_SIZE PAGE_SIZE
#ifdef CONFIG_MICROCODE_AMD
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index b429d3f554b9..2df366cc71ce 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -605,6 +605,10 @@ static unsigned int verify_patch_size(u8 family, u32
patch_size, unsigned int si
{
u32 max_size;
+/*
+ * If you modify these values or add a new one also check whether
+ * PATCH_MAX_SIZE in include/asm/microcode_amd.h needs updating, too.
+ */
#define F1XH_MPB_MAX_SIZE 2048
#define F14H_MPB_MAX_SIZE 1824
#define F15H_MPB_MAX_SIZE 4096
[PATCH v5 6/6] x86/microcode/AMD: Check the equivalence table size when scanning it
Currently, the code scanning the CPU equivalence table read from a
microcode container file assumes that it actually contains a terminating
zero entry.
Let's check also the size of this table to make sure that we don't read
past it in case it actually doesn't.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
arch/x86/kernel/cpu/microcode/amd.c | 33 ++---
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 2df366cc71ce..5ba933ce1d51 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -39,6 +39,7 @@
#include
static struct equiv_cpu_entry *equiv_cpu_table;
+static unsigned int equiv_cpu_table_entries;
/*
* This points to the current valid container of microcode patches which we
will
@@ -63,12 +64,18 @@ static u8 amd_ucode_patch[PATCH_MAX_SIZE];
static const char
ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";
-static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
+static u16 find_equiv_id(const struct equiv_cpu_entry *equiv_table,
+unsigned int equiv_table_entries, u32 sig)
{
- for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
- if (sig == equiv_table->installed_cpu)
- return equiv_table->equiv_cpu;
- }
+ unsigned int i;
+
+ if (!equiv_table)
+ return 0;
+
+ for (i = 0; i < equiv_table_entries && equiv_table[i].installed_cpu;
+i++)
+ if (sig == equiv_table[i].installed_cpu)
+ return equiv_table[i].equiv_cpu;
return 0;
}
@@ -237,7 +244,8 @@ static size_t parse_container(u8 *ucode, size_t size,
struct cont_desc *desc)
eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
/* Find the equivalence ID of our CPU in this table: */
- eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
+ eq_id = find_equiv_id(eq, equiv_tbl_len / sizeof(*eq),
+ desc->cpuid_1_eax);
buf += CONTAINER_HDR_SZ;
buf += equiv_tbl_len;
@@ -499,20 +507,21 @@ void reload_ucode_amd(void)
static u16 __find_equiv_id(unsigned int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- return find_equiv_id(equiv_cpu_table, uci->cpu_sig.sig);
+ return find_equiv_id(equiv_cpu_table, equiv_cpu_table_entries,
+uci->cpu_sig.sig);
}
static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
{
- int i = 0;
+ unsigned int i;
BUG_ON(!equiv_cpu_table);
- while (equiv_cpu_table[i].equiv_cpu != 0) {
+ for (i = 0; i < equiv_cpu_table_entries &&
+equiv_cpu_table[i].equiv_cpu != 0; i++)
if (equiv_cpu == equiv_cpu_table[i].equiv_cpu)
return equiv_cpu_table[i].installed_cpu;
- i++;
- }
+
return 0;
}
@@ -697,6 +706,7 @@ static unsigned int install_equiv_cpu_table(const u8 *buf,
size_t buf_size)
}
memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
+ equiv_cpu_table_entries = equiv_tbl_len / sizeof(struct
equiv_cpu_entry);
return equiv_tbl_len;
}
@@ -705,6 +715,7 @@ static void free_equiv_cpu_table(void)
{
vfree(equiv_cpu_table);
equiv_cpu_table = NULL;
+ equiv_cpu_table_entries = 0;
}
static void cleanup(void)
[PATCH v5 2/6] x86/microcode/AMD: Add microcode container data checking functions
This commit adds verify_container(), verify_equivalence_table(),
verify_patch_section() and verify_patch() functions to the AMD microcode
update driver.
These functions check whether a passed buffer contains the relevant
structure, whether it isn't truncated and (for actual microcode patches)
whether the size of a patch is not too large for a particular CPU family.
By adding these checks as separate functions the actual microcode loading
code won't get interspersed with a lot of checks and so will be more
readable.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
arch/x86/kernel/cpu/microcode/amd.c | 140 +++-
1 file changed, 137 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index dc8ea9a9d962..4fafaf0852d7 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -73,6 +73,142 @@ static u16 find_equiv_id(struct equiv_cpu_entry
*equiv_table, u32 sig)
return 0;
}
+/*
+ * Checks whether there is a valid microcode container file at the beginning
+ * of a passed buffer @buf of size @size.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_container(const u8 *buf, size_t buf_size, bool early)
+{
+ u32 cont_magic;
+
+ if (buf_size <= CONTAINER_HDR_SZ) {
+ if (!early)
+ pr_err("Truncated microcode container header.\n");
+
+ return false;
+ }
+
+ cont_magic = *(const u32 *)buf;
+ if (cont_magic != UCODE_MAGIC) {
+ if (!early)
+ pr_err("Invalid magic value (0x%08x).\n", cont_magic);
+
+ return false;
+ }
+
+ return true;
+}
+
+/*
+ * Checks whether there is a valid, non-truncated CPU equivalence table
+ * at the beginning of a passed buffer @buf of size @size.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_equivalence_table(const u8 *buf, size_t buf_size, bool
early)
+{
+ const u32 *hdr = (const u32 *)buf;
+ u32 cont_type, equiv_tbl_len;
+
+ cont_type = hdr[1];
+ if (cont_type != UCODE_EQUIV_CPU_TABLE_TYPE) {
+ if (!early)
+ pr_err("Wrong microcode container equivalence table
type: %u.\n",
+ cont_type);
+
+ return false;
+ }
+
+ equiv_tbl_len = hdr[2];
+ if (equiv_tbl_len < sizeof(struct equiv_cpu_entry) ||
+ buf_size - CONTAINER_HDR_SZ < equiv_tbl_len) {
+ if (!early)
+ pr_err("Truncated equivalence table.\n");
+
+ return false;
+ }
+
+ return true;
+}
+
+/*
+ * Checks whether there is a valid, non-truncated microcode patch section
+ * at the beginning of a passed buffer @buf of size @size.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_patch_section(const u8 *buf, size_t buf_size, bool early)
+{
+ const u32 *hdr = (const u32 *)buf;
+ u32 patch_type, patch_size;
+
+ if (buf_size < SECTION_HDR_SIZE) {
+ if (!early)
+ pr_err("Truncated patch section.\n");
+
+ return false;
+ }
+
+ patch_type = hdr[0];
+ patch_size = hdr[1];
+
+ if (patch_type != UCODE_UCODE_TYPE) {
+ if (!early)
+ pr_err("Invalid type field (%u) in container file
section header.\n",
+ patch_type);
+
+ return false;
+ }
+
+ if (patch_size < sizeof(struct microcode_header_amd)) {
+ if (!early)
+ pr_err("Patch of size %u too short.\n", patch_size);
+
+ return false;
+ }
+
+ if (buf_size - SECTION_HDR_SIZE < patch_size) {
+ if (!early)
+ pr_err("Patch of size %u truncated.\n", patch_size);
+
+ return false;
+ }
+
+ return true;
+}
+
+static unsigned int verify_patch_size(u8 family, u32 patch_size,
+ unsigned int size);
+
+/*
+ * Checks whether a microcode patch located at the beginning of a passed
+ * buffer @buf of size @size is not too large for a particular @family
+ * and is not truncated.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_patch(u8 family, const u8 *buf, size_t buf_size, bool early)
+{
+ const u32 *hdr = (const u32 *)buf;
+ u32 patch_size = hdr[1];
+
+ /*
+* The section header length is not incl
[PATCH v5 3/6] x86/microcode/AMD: Check microcode container data in the early loader
This commit converts the early loader in the AMD microcode update driver to
use the container data checking functions introduced by the previous
commit.
We have to be careful to call these functions with 'early' parameter set,
so they won't try to print errors as the early loader runs too early for
printk()-style functions to work.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
arch/x86/kernel/cpu/microcode/amd.c | 45 ++---
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 4fafaf0852d7..94fcd702a67a 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -216,29 +216,33 @@ static bool verify_patch(u8 family, const u8 *buf, size_t
buf_size, bool early)
* Returns the amount of bytes consumed while scanning. @desc contains all the
* data we're going to use in later stages of the application.
*/
-static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
+static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
{
struct equiv_cpu_entry *eq;
- ssize_t orig_size = size;
+ size_t orig_size = size;
u32 *hdr = (u32 *)ucode;
+ u32 equiv_tbl_len;
u16 eq_id;
u8 *buf;
- /* Am I looking at an equivalence table header? */
- if (hdr[0] != UCODE_MAGIC ||
- hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
- hdr[2] == 0)
+ if (!verify_container(ucode, size, true))
+ return 0;
+
+ if (!verify_equivalence_table(ucode, size, true))
return CONTAINER_HDR_SZ;
buf = ucode;
+ equiv_tbl_len = hdr[2];
eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
/* Find the equivalence ID of our CPU in this table: */
eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
- buf += hdr[2] + CONTAINER_HDR_SZ;
- size -= hdr[2] + CONTAINER_HDR_SZ;
+ buf += CONTAINER_HDR_SZ;
+ buf += equiv_tbl_len;
+ size -= CONTAINER_HDR_SZ;
+ size -= equiv_tbl_len;
/*
* Scan through the rest of the container to find where it ends. We do
@@ -250,25 +254,22 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
hdr = (u32 *)buf;
- if (hdr[0] != UCODE_UCODE_TYPE)
+ if (!verify_patch_section(buf, size, true))
break;
- /* Sanity-check patch size. */
patch_size = hdr[1];
- if (patch_size > PATCH_MAX_SIZE)
- break;
- /* Skip patch section header: */
- buf += SECTION_HDR_SIZE;
- size -= SECTION_HDR_SIZE;
-
- mc = (struct microcode_amd *)buf;
- if (eq_id == mc->hdr.processor_rev_id) {
+ mc = (struct microcode_amd *)(buf + SECTION_HDR_SIZE);
+ if (eq_id == mc->hdr.processor_rev_id &&
+ verify_patch(x86_family(desc->cpuid_1_eax), buf, size,
+true)) {
desc->psize = patch_size;
desc->mc = mc;
}
+ buf += SECTION_HDR_SIZE;
buf += patch_size;
+ size -= SECTION_HDR_SIZE;
size -= patch_size;
}
@@ -295,15 +296,13 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
*/
static void scan_containers(u8 *ucode, size_t size, struct cont_desc *desc)
{
- ssize_t rem = size;
-
- while (rem >= 0) {
- ssize_t s = parse_container(ucode, rem, desc);
+ while (size > 0) {
+ size_t s = parse_container(ucode, size, desc);
if (!s)
return;
ucode += s;
- rem -= s;
+ size -= s;
}
}
[PATCH v5 0/6] x86/microcode/AMD: Check microcode file sanity before loading it
Currently, it is very easy to make the AMD microcode update driver crash
or spin on a malformed microcode container file since it does very little
consistency checking on data loaded from such file.
This series introduces various checks, mostly on length-type fields,
so all corrupted microcode container files are (hopefully) correctly
rejected instead.
This largely matches what the Intel microcode update driver already does.
It also tries to make the behavior consistent between the early and late
loaders.
Please note that this isn't about verifying the actual microcode update,
that is, the blob that gets sent to the CPU as the new microcode.
Such verification is (hopefully) done by the CPU itself.
It is about verifying a driver-specific container file that includes
many microcode updates for different CPUs of a particular CPU family,
along with metadata that helps the driver select the right microcode
update to actually send to the CPU.
There are purposely-corrupted test files available at
https://pastebin.com/XkKUSmMp
One has to enable KASAN in the kernel config and rename a particular
test file to name appropriate to the running CPU family to test its
loading.
Changes from v1:
* Capitalize a comment,
* Rename 'eqsize' and 'bufsize' to 'eq_size' and 'buf_size',
respectively,
* Attach a comment about checking the equivalence table header to its
first size check,
* Rename 'equiv{_cpu,}_table_size' to 'equiv{_cpu,}_table_entries'.
Changes from v2:
* Split the patch into separate commits,
* Remove explicit CPU equivalence table size limit,
* Make install_equiv_cpu_table() return a size_t instead of a (signed)
int so no overflow can occur there,
* Automatically compute the PATCH_MAX_SIZE macro and use it for checking
a patch size,
* Make the late loader behavior with respect to late parse failures
consistent with what the early loader does.
Changes from v3:
* Capitalize patch subject names,
* Add a comment describing why we subtract SECTION_HDR_SIZE from a file
leftover length before calling verify_patch_size(),
* Don't break a long line containing the above subtraction,
* Move a comment in parse_container() back to where it was in the original
patch version,
* Add special local vars for container header fields in parse_container(),
* Return the remaining blob size from parse_container() if the equivalence
table is truncated,
* Split the equivalence table size checks into two patches: one for the
early loader and one for the late loader,
* Rename an equivalence table length variable in install_equiv_cpu_table()
for consistency with a similar one in parse_container(),
* Rephrase the error messages in install_equiv_cpu_table() to be more
descriptive and merge two tests there so they print the same message,
* Change install_equiv_cpu_table() to return an unsigned int while moving
the job of adding the container header length to this value to its caller,
* Drop automatic computation of the PATCH_MAX_SIZE macro and add a comment
reminding to do it manually instead,
* Add a local variable patch_type for better code readability in
verify_and_add_patch() function.
Changes from v4:
* Update the first patch in series with Borislav's version,
* Use u32-type variables for microcode container header fields,
* Drop the check for zero-length CPU equivalence table,
* Leave size variables in verify_patch_size() as unsigned ints,
* Rewrite the series to use common microcode container data checking
functions in both the early and the late loader so their code won't get
interspersed with a lot of checks and so will be more readable.
arch/x86/include/asm/microcode_amd.h | 1 +
arch/x86/kernel/cpu/microcode/amd.c | 312 ++-
2 files changed, 232 insertions(+), 81 deletions(-)
[PATCH v5 1/6] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length
verify_patch_size() verifies whether the remaining size of the microcode
container file is large enough to contain a patch of the indicated size.
However, the section header length is not included in this indicated
size but it is present in the leftover file length so it should be
subtracted from the leftover file length before passing this value to
verify_patch_size().
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
Link:
http://lkml.kernel.org/r/b4854b17-e3ba-54d6-488d-0e0bfffe4...@maciej.szmigiero.name
[ Split comment. ]
Signed-off-by: Borislav Petkov <b...@suse.de>
---
arch/x86/kernel/cpu/microcode/amd.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 0624957aa068..dc8ea9a9d962 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -461,8 +461,12 @@ static int collect_cpu_info_amd(int cpu, struct
cpu_signature *csig)
return 0;
}
-static unsigned int verify_patch_size(u8 family, u32 patch_size,
- unsigned int size)
+/*
+ * Check whether the passed remaining file @size is large enough to contain a
+ * patch of the indicated @patch_size (and also whether this size does not
+ * exceed the per-family maximum).
+ */
+static unsigned int verify_patch_size(u8 family, u32 patch_size, unsigned int
size)
{
u32 max_size;
@@ -613,7 +617,12 @@ static int verify_and_add_patch(u8 family, u8 *fw,
unsigned int leftover)
return crnt_size;
}
- ret = verify_patch_size(family, patch_size, leftover);
+ /*
+* The section header length is not included in this indicated size
+* but is present in the leftover file length so we need to subtract
+* it before passing this value to the function below.
+*/
+ ret = verify_patch_size(family, patch_size, leftover -
SECTION_HDR_SIZE);
if (!ret) {
pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
return crnt_size;
[PATCH v2] X.509: unpack RSA signatureValue field from BIT STRING
The signatureValue field of a X.509 certificate is encoded as a BIT STRING.
For RSA signatures this BIT STRING is of so-called primitive subtype, which
contains a u8 prefix indicating a count of unused bits in the encoding.
We have to strip this prefix from signature data, just as we already do for
key data in x509_extract_key_data() function.
This wasn't noticed earlier because this prefix byte is zero for RSA key
sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero
prefixes has no bearing on its value.
The signature length, however was incorrect, which is a problem for RSA
implementations that need it to be exactly correct (like AMD CCP).
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
Fixes: c26fd69fa009 ("X.509: Add a crypto key parser for binary (DER) X.509
certificates")
Cc: sta...@vger.kernel.org
---
This is a resend of a patch that was previously submitted in one series
with CCP driver changes since this particular patch should go through
the security (rather than crypto) tree.
Changes from v1: Change '!' to '== 0'.
crypto/asymmetric_keys/x509_cert_parser.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
b/crypto/asymmetric_keys/x509_cert_parser.c
index 7d81e6bb461a..b6cabac4b62b 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -249,6 +249,15 @@ int x509_note_signature(void *context, size_t hdrlen,
return -EINVAL;
}
+ if (strcmp(ctx->cert->sig->pkey_algo, "rsa") == 0) {
+ /* Discard the BIT STRING metadata */
+ if (vlen < 1 || *(const u8 *)value != 0)
+ return -EBADMSG;
+
+ value++;
+ vlen--;
+ }
+
ctx->cert->raw_sig = value;
ctx->cert->raw_sig_size = vlen;
return 0;
[PATCH v6 6/6] [media] cxusb: add analog mode support for Medion MD95700
This patch adds support for analog part of Medion 95700 in the cxusb
driver.
What works:
* Video capture at various sizes with sequential fields,
* Input switching (TV Tuner, Composite, S-Video),
* TV and radio tuning,
* Video standard switching and auto detection,
* Radio mode switching (stereo / mono),
* Unplugging while capturing,
* DVB / analog coexistence,
* Raw BT.656 stream support.
What does not work yet:
* Audio,
* VBI,
* Picture controls.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
drivers/media/usb/dvb-usb/Kconfig| 16 +-
drivers/media/usb/dvb-usb/Makefile |3 +
drivers/media/usb/dvb-usb/cxusb-analog.c | 1914 ++
drivers/media/usb/dvb-usb/cxusb.c|2 -
drivers/media/usb/dvb-usb/cxusb.h| 106 ++
5 files changed, 2037 insertions(+), 4 deletions(-)
create mode 100644 drivers/media/usb/dvb-usb/cxusb-analog.c
diff --git a/drivers/media/usb/dvb-usb/Kconfig
b/drivers/media/usb/dvb-usb/Kconfig
index 2651ae277347..b4ef8f6eb470 100644
--- a/drivers/media/usb/dvb-usb/Kconfig
+++ b/drivers/media/usb/dvb-usb/Kconfig
@@ -138,12 +138,24 @@ config DVB_USB_CXUSB
select MEDIA_TUNER_SI2157 if MEDIA_SUBDRV_AUTOSELECT
help
Say Y here to support the Conexant USB2.0 hybrid reference design.
- Currently, only DVB and ATSC modes are supported, analog mode
- shall be added in the future. Devices that require this module:
+ DVB and ATSC modes are supported, for a basic analog mode support
+ see the next option ("Analog support for the Conexant USB2.0 hybrid
+ reference design").
+ Devices that require this module:
Medion MD95700 hybrid USB2.0 device.
DViCO FusionHDTV (Bluebird) USB2.0 devices
+config DVB_USB_CXUSB_ANALOG
+ bool "Analog support for the Conexant USB2.0 hybrid reference design"
+ depends on DVB_USB_CXUSB && VIDEO_V4L2
+ select VIDEO_CX25840
+ select VIDEOBUF2_VMALLOC
+ help
+ Say Y here to enable basic analog mode support for the Conexant
+ USB2.0 hybrid reference design.
+ Currently this mode is supported only on a Medion MD95700 device.
+
config DVB_USB_M920X
tristate "Uli m920x DVB-T USB2.0 support"
depends on DVB_USB
diff --git a/drivers/media/usb/dvb-usb/Makefile
b/drivers/media/usb/dvb-usb/Makefile
index 9ad2618408ef..e47bcadcfc3d 100644
--- a/drivers/media/usb/dvb-usb/Makefile
+++ b/drivers/media/usb/dvb-usb/Makefile
@@ -42,6 +42,9 @@ dvb-usb-digitv-objs := digitv.o
obj-$(CONFIG_DVB_USB_DIGITV) += dvb-usb-digitv.o
dvb-usb-cxusb-objs := cxusb.o
+ifeq ($(CONFIG_DVB_USB_CXUSB_ANALOG),y)
+dvb-usb-cxusb-objs += cxusb-analog.o
+endif
obj-$(CONFIG_DVB_USB_CXUSB) += dvb-usb-cxusb.o
dvb-usb-ttusb2-objs := ttusb2.o
diff --git a/drivers/media/usb/dvb-usb/cxusb-analog.c
b/drivers/media/usb/dvb-usb/cxusb-analog.c
new file mode 100644
index ..969d82b24f41
--- /dev/null
+++ b/drivers/media/usb/dvb-usb/cxusb-analog.c
@@ -0,0 +1,1914 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// DVB USB compliant linux driver for Conexant USB reference design -
+// (analog part).
+//
+// Copyright (C) 2011, 2017 Maciej S. Szmigiero (m...@maciej.szmigiero.name)
+//
+// TODO:
+// * audio support,
+// * finish radio support (requires audio of course),
+// * VBI support,
+// * controls support
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "cxusb.h"
+
+static int cxusb_medion_v_queue_setup(struct vb2_queue *q,
+ unsigned int *num_buffers,
+ unsigned int *num_planes,
+ unsigned int sizes[],
+ struct device *alloc_devs[])
+{
+ struct dvb_usb_device *dvbdev = vb2_get_drv_priv(q);
+ struct cxusb_medion_dev *cxdev = dvbdev->priv;
+ unsigned int size = cxdev->raw_mode ?
+ CXUSB_VIDEO_MAX_FRAME_SIZE :
+ cxdev->width * cxdev->height * 2;
+
+ if (*num_planes > 0) {
+ if (*num_planes != 1)
+ return -EINVAL;
+
+ if (sizes[0] < size)
+ return -EINVAL;
+ } else {
+ *num_planes = 1;
+ sizes[0] = size;
+ }
+
+ if (q->num_buffers + *num_buffers < 6)
+ *num_buffers = 6 - q->num_buffers;
+
+ return 0;
+}
+
+static int cxusb_medion_v_buf_init(struct vb2_buffer *vb)
+{
+ struct dvb_usb_device *dvbdev = vb2_get_drv_priv(vb->vb2_queue);
+ struct cxusb_medion_dev *cxdev = dvbdev->priv;
+
+ cxusb_vprintk(dvbdev, OPS, "buffer init\n");
+
+ if (cxdev->raw_mode) {
+ if (vb2_pl
[PATCH v6 3/6] cx25840: add pin to pad mapping and output format configuration
This commit adds pin to pad mapping and output format configuration support
in CX2584x-series chips to cx25840 driver.
This functionality is then used to allow disabling ivtv-specific hacks
(called a "generic mode"), so cx25840 driver can be used for other devices
not needing them without risking compatibility problems.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
drivers/media/i2c/cx25840/cx25840-core.c | 396 ++-
drivers/media/i2c/cx25840/cx25840-core.h | 13 +
drivers/media/i2c/cx25840/cx25840-vbi.c | 3 +
include/media/drv-intf/cx25840.h | 74 +-
4 files changed, 484 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/cx25840/cx25840-core.c
b/drivers/media/i2c/cx25840/cx25840-core.c
index b168bf3635b6..7dc3d0870808 100644
--- a/drivers/media/i2c/cx25840/cx25840-core.c
+++ b/drivers/media/i2c/cx25840/cx25840-core.c
@@ -21,6 +21,9 @@
* CX23888 DIF support for the HVR1850
* Copyright (C) 2011 Steven Toth <st...@kernellabs.com>
*
+ * CX2584x pin to pad mapping and output format configuration support are
+ * Copyright (C) 2011 Maciej S. Szmigiero <m...@maciej.szmigiero.name>
+ *
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
@@ -316,6 +319,260 @@ static int cx23885_s_io_pin_config(struct v4l2_subdev
*sd, size_t n,
return 0;
}
+static u8 cx25840_function_to_pad(struct i2c_client *client, u8 function)
+{
+ switch (function) {
+ case CX25840_PAD_ACTIVE:
+ return 1;
+
+ case CX25840_PAD_VACTIVE:
+ return 2;
+
+ case CX25840_PAD_CBFLAG:
+ return 3;
+
+ case CX25840_PAD_VID_DATA_EXT0:
+ return 4;
+
+ case CX25840_PAD_VID_DATA_EXT1:
+ return 5;
+
+ case CX25840_PAD_GPO0:
+ return 6;
+
+ case CX25840_PAD_GPO1:
+ return 7;
+
+ case CX25840_PAD_GPO2:
+ return 8;
+
+ case CX25840_PAD_GPO3:
+ return 9;
+
+ case CX25840_PAD_IRQ_N:
+ return 10;
+
+ case CX25840_PAD_AC_SYNC:
+ return 11;
+
+ case CX25840_PAD_AC_SDOUT:
+ return 12;
+
+ case CX25840_PAD_PLL_CLK:
+ return 13;
+
+ case CX25840_PAD_VRESET:
+ return 14;
+
+ default:
+ if (function != CX25840_PAD_DEFAULT)
+ v4l_err(client,
+ "invalid function %u, assuming default\n",
+ (unsigned int)function);
+ return 0;
+ }
+}
+
+static void cx25840_set_invert(u8 *pinctrl3, u8 *voutctrl4, u8 function,
+ u8 pin, bool invert)
+{
+ switch (function) {
+ case CX25840_PAD_IRQ_N:
+ if (invert)
+ *pinctrl3 &= ~2;
+ else
+ *pinctrl3 |= 2;
+ break;
+
+ case CX25840_PAD_ACTIVE:
+ if (invert)
+ *voutctrl4 |= BIT(2);
+ else
+ *voutctrl4 &= ~BIT(2);
+ break;
+
+ case CX25840_PAD_VACTIVE:
+ if (invert)
+ *voutctrl4 |= BIT(5);
+ else
+ *voutctrl4 &= ~BIT(5);
+ break;
+
+ case CX25840_PAD_CBFLAG:
+ if (invert)
+ *voutctrl4 |= BIT(4);
+ else
+ *voutctrl4 &= ~BIT(4);
+ break;
+
+ case CX25840_PAD_VRESET:
+ if (invert)
+ *voutctrl4 |= BIT(0);
+ else
+ *voutctrl4 &= ~BIT(0);
+ break;
+ }
+
+ if (function != CX25840_PAD_DEFAULT)
+ return;
+
+ switch (pin) {
+ case CX25840_PIN_DVALID_PRGM0:
+ if (invert)
+ *voutctrl4 |= BIT(6);
+ else
+ *voutctrl4 &= ~BIT(6);
+ break;
+
+ case CX25840_PIN_HRESET_PRGM2:
+ if (invert)
+ *voutctrl4 |= BIT(1);
+ else
+ *voutctrl4 &= ~BIT(1);
+ break;
+ }
+}
+
+static int cx25840_s_io_pin_config(struct v4l2_subdev *sd, size_t n,
+ struct v4l2_subdev_io_pin_config *p)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ unsigned int i;
+ u8 pinctrl[6], pinconf[10], voutctrl4;
+
+ for (i = 0; i < 6; i++)
+ pinctrl[i] = cx25840_read(client, 0x114 + i);
+
+ for (i = 0; i < 10; i++)
+ pinconf[i] = cx25840_read(client, 0x11c + i);
+
+
[PATCH v6 5/6] [media] cxusb: implement Medion MD95700 digital / analog coexistence
This patch prepares cxusb driver for supporting the analog part of
Medion 95700 (previously only the digital - DVB - mode was supported).
Specifically, it adds support for:
* switching the device between analog and digital modes of operation,
* enforcing that only one mode is active at the same time due to hardware
limitations.
Actual implementation of the analog mode will be provided by the next
commit.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
drivers/media/usb/dvb-usb/cxusb.c| 450 +++
drivers/media/usb/dvb-usb/cxusb.h| 48
drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 20 +-
drivers/media/usb/dvb-usb/dvb-usb-init.c | 13 +
drivers/media/usb/dvb-usb/dvb-usb.h | 8 +
5 files changed, 486 insertions(+), 53 deletions(-)
diff --git a/drivers/media/usb/dvb-usb/cxusb.c
b/drivers/media/usb/dvb-usb/cxusb.c
index 387a074ea6ec..11df020b7de3 100644
--- a/drivers/media/usb/dvb-usb/cxusb.c
+++ b/drivers/media/usb/dvb-usb/cxusb.c
@@ -16,6 +16,7 @@
* Copyright (C) 2005 Patrick Boettcher (patrick.boettc...@posteo.de)
* Copyright (C) 2006 Michael Krufky (mkru...@linuxtv.org)
* Copyright (C) 2006, 2007 Chris Pascoe (c.pas...@itee.uq.edu.au)
+ * Copyright (C) 2011, 2017 Maciej S. Szmigiero (m...@maciej.szmigiero.name)
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License as published by the Free
@@ -24,9 +25,12 @@
* see Documentation/dvb/README.dvb-usb for more information
*/
#include
-#include
-#include
+#include
+#include
#include
+#include
+#include
+#include
#include "cxusb.h"
@@ -47,17 +51,45 @@
#include "si2157.h"
/* debug */
-static int dvb_usb_cxusb_debug;
+int dvb_usb_cxusb_debug;
module_param_named(debug, dvb_usb_cxusb_debug, int, 0644);
-MODULE_PARM_DESC(debug, "set debugging level (1=rc (or-able))."
DVB_USB_DEBUG_STATUS);
+MODULE_PARM_DESC(debug, "set debugging level (see cxusb.h)."
+DVB_USB_DEBUG_STATUS);
DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
-#define deb_info(args...) dprintk(dvb_usb_cxusb_debug, 0x03, args)
-#define deb_i2c(args...)dprintk(dvb_usb_cxusb_debug, 0x02, args)
+#define deb_info(args...) dprintk(dvb_usb_cxusb_debug, CXUSB_DBG_MISC, args)
+#define deb_i2c(args...)dprintk(dvb_usb_cxusb_debug, CXUSB_DBG_I2C, args)
+
+enum cxusb_table_index {
+ MEDION_MD95700,
+ DVICO_BLUEBIRD_LG064F_COLD,
+ DVICO_BLUEBIRD_LG064F_WARM,
+ DVICO_BLUEBIRD_DUAL_1_COLD,
+ DVICO_BLUEBIRD_DUAL_1_WARM,
+ DVICO_BLUEBIRD_LGZ201_COLD,
+ DVICO_BLUEBIRD_LGZ201_WARM,
+ DVICO_BLUEBIRD_TH7579_COLD,
+ DVICO_BLUEBIRD_TH7579_WARM,
+ DIGITALNOW_BLUEBIRD_DUAL_1_COLD,
+ DIGITALNOW_BLUEBIRD_DUAL_1_WARM,
+ DVICO_BLUEBIRD_DUAL_2_COLD,
+ DVICO_BLUEBIRD_DUAL_2_WARM,
+ DVICO_BLUEBIRD_DUAL_4,
+ DVICO_BLUEBIRD_DVB_T_NANO_2,
+ DVICO_BLUEBIRD_DVB_T_NANO_2_NFW_WARM,
+ AVERMEDIA_VOLAR_A868R,
+ DVICO_BLUEBIRD_DUAL_4_REV_2,
+ CONEXANT_D680_DMB,
+ MYGICA_D689,
+ MYGICA_T230,
+ NR__cxusb_table_index
+};
+
+static struct usb_device_id cxusb_table[];
-static int cxusb_ctrl_msg(struct dvb_usb_device *d,
- u8 cmd, const u8 *wbuf, int wlen, u8 *rbuf, int rlen)
+int cxusb_ctrl_msg(struct dvb_usb_device *d,
+ u8 cmd, const u8 *wbuf, int wlen, u8 *rbuf, int rlen)
{
struct cxusb_state *st = d->priv;
int ret;
@@ -89,7 +121,8 @@ static void cxusb_gpio_tuner(struct dvb_usb_device *d, int
onoff)
struct cxusb_state *st = d->priv;
u8 o[2], i;
- if (st->gpio_write_state[GPIO_TUNER] == onoff)
+ if (st->gpio_write_state[GPIO_TUNER] == onoff &&
+ !st->gpio_write_refresh[GPIO_TUNER])
return;
o[0] = GPIO_TUNER;
@@ -100,6 +133,7 @@ static void cxusb_gpio_tuner(struct dvb_usb_device *d, int
onoff)
deb_info("gpio_write failed.\n");
st->gpio_write_state[GPIO_TUNER] = onoff;
+ st->gpio_write_refresh[GPIO_TUNER] = false;
}
static int cxusb_bluebird_gpio_rw(struct dvb_usb_device *d, u8 changemask,
@@ -259,7 +293,7 @@ static int cxusb_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg msg[],
static u32 cxusb_i2c_func(struct i2c_adapter *adapter)
{
- return I2C_FUNC_I2C;
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
}
static struct i2c_algorithm cxusb_i2c_algo = {
@@ -267,15 +301,48 @@ static struct i2c_algorithm cxusb_i2c_algo = {
.functionality = cxusb_i2c_func,
};
-static int cxusb_power_ctrl(struct dvb_usb_device *d, int onoff)
+static int _cxusb_power_ctrl(struct dvb_usb_device *d, int onoff)
{
u8 b = 0;
+
+ deb_info("setting power %s\n", onoff ? "ON" : "OFF");
+
if (onoff)
[PATCH v6 2/6] cx25840: add kernel-doc description of struct cx25840_state
This commit describes a device instance private data of the driver
(struct cx25840_state) in a kernel-doc style comment.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
drivers/media/i2c/cx25840/cx25840-core.h | 33 ++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/cx25840/cx25840-core.h
b/drivers/media/i2c/cx25840/cx25840-core.h
index fb13a624d2e3..c323b1af1f83 100644
--- a/drivers/media/i2c/cx25840/cx25840-core.h
+++ b/drivers/media/i2c/cx25840/cx25840-core.h
@@ -45,6 +45,35 @@ enum cx25840_media_pads {
CX25840_NUM_PADS
};
+/**
+ * struct cx25840_state - a device instance private data
+ * @c: i2c_client struct representing this device
+ * @sd:our V4L2 sub-device
+ * @hdl: our V4L2 control handler
+ * @volume:audio volume V4L2 control (non-cx2583x devices only)
+ * @mute: audio mute V4L2 control (non-cx2583x devices only)
+ * @pvr150_workaround: whether we enable workaround for Hauppauge PVR150
+ * hardware bug (audio dropping out)
+ * @radio: set if we are currently in the radio mode, otherwise
+ * the current mode is non-radio (that is, video)
+ * @std: currently set video standard
+ * @vid_input: currently set video input
+ * @aud_input: currently set audio input
+ * @audclk_freq: currently set audio sample rate
+ * @audmode: currently set audio mode (when in non-radio mode)
+ * @vbi_line_offset: vbi line number offset
+ * @id:exact device model
+ * @rev: raw device id read from the chip
+ * @is_initialized:whether we have already loaded firmware into the chip
+ * and initialized it
+ * @vbi_regs_offset: offset of vbi regs
+ * @fw_wait: wait queue to wake an initalization function up when
+ * firmware loading (on a separate workqueue) finishes
+ * @fw_work: a work that actually loads the firmware on a separate
+ * workqueue
+ * @ir_state: a pointer to chip IR controller private data
+ * @pads: array of supported chip pads (currently only a stub)
+ */
struct cx25840_state {
struct i2c_client *c;
struct v4l2_subdev sd;
@@ -66,8 +95,8 @@ struct cx25840_state {
u32 rev;
int is_initialized;
unsigned vbi_regs_offset;
- wait_queue_head_t fw_wait;/* wake up when the fw load is finished */
- struct work_struct fw_work; /* work entry for fw load */
+ wait_queue_head_t fw_wait;
+ struct work_struct fw_work;
struct cx25840_ir_state *ir_state;
#if defined(CONFIG_MEDIA_CONTROLLER)
struct media_padpads[CX25840_NUM_PADS];
[PATCH v6 4/6] tuner-simple: allow setting mono radio mode
For some types of tuners (Philips FMD1216ME(X) MK3 currently) we know that letting TDA9887 output port 1 remain high (inactive) will switch FM radio to mono mode. Let's make use of this functionality - nothing changes for the default stereo radio mode. Tested on a Medion 95700 board which has a FMD1216ME tuner. Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name> --- drivers/media/tuners/tuner-simple.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/tuners/tuner-simple.c b/drivers/media/tuners/tuner-simple.c index 36b88f820239..29c1473f2e9f 100644 --- a/drivers/media/tuners/tuner-simple.c +++ b/drivers/media/tuners/tuner-simple.c @@ -670,6 +670,7 @@ static int simple_set_radio_freq(struct dvb_frontend *fe, int rc, j; struct tuner_params *t_params; unsigned int freq = params->frequency; + bool mono = params->audmode == V4L2_TUNER_MODE_MONO; tun = priv->tun; @@ -736,8 +737,8 @@ static int simple_set_radio_freq(struct dvb_frontend *fe, config |= TDA9887_PORT2_ACTIVE; if (t_params->intercarrier_mode) config |= TDA9887_INTERCARRIER; -/* if (t_params->port1_set_for_fm_mono) - config &= ~TDA9887_PORT1_ACTIVE;*/ + if (t_params->port1_set_for_fm_mono && mono) + config &= ~TDA9887_PORT1_ACTIVE; if (t_params->fm_gain_normal) config |= TDA9887_GAIN_NORMAL; if (t_params->radio_if == 2)
[PATCH v6 1/6] ivtv: zero-initialize cx25840 platform data
We need to zero-initialize cx25840 platform data structure to make sure that its future members do not contain random stack garbage. Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name> --- drivers/media/pci/ivtv/ivtv-i2c.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/pci/ivtv/ivtv-i2c.c b/drivers/media/pci/ivtv/ivtv-i2c.c index 522cd111e399..e9ce54dd5e01 100644 --- a/drivers/media/pci/ivtv/ivtv-i2c.c +++ b/drivers/media/pci/ivtv/ivtv-i2c.c @@ -293,6 +293,7 @@ int ivtv_i2c_register(struct ivtv *itv, unsigned idx) .platform_data = , }; + memset(, 0, sizeof(pdata)); pdata.pvr150_workaround = itv->pvr150_workaround; sd = v4l2_i2c_new_subdev_board(>v4l2_dev, adap, _info, NULL);
[PATCH v6 0/6] [media] Add analog mode support for Medion MD95700
This series adds support for analog part of Medion 95700 in the cxusb driver. What works: * Video capture at various sizes with sequential fields, * Input switching (TV Tuner, Composite, S-Video), * TV and radio tuning, * Video standard switching and auto detection, * Radio mode switching (stereo / mono), * Unplugging while capturing, * DVB / analog coexistence, * Raw BT.656 stream support. What does not work yet: * Audio, * VBI, * Picture controls. This series (as a one patch) was submitted for inclusion few years ago, then waited few months in a patch queue. Unfortunately, by the time it was supposed to be merged there were enough changes in media that it was no longer mergable. I thought at that time that I will be able to rebase and retest it soon but unfortunately up till now I was never able to find enough time to do so. Also, with the passing of time the implementation diverged more and more from the current kernel code, necessitating even more reworking. That last iteration can be found here: https://patchwork.linuxtv.org/patch/8048/ Since that version there had been the following changes: * Adaptation to changes in V4L2 / DVB core, * Radio device was added, with a possibility to tune to a FM radio station and switch between stereo and mono modes (tested by taping audio signal directly at tuner output pin), * DVB / analog coexistence was improved - resolved a few cases where DVB core would switch off power or reset the tuner when the device was still being used but in the analog mode, * Fixed issues reported by v4l2-compliance, * Switching to raw BT.656 mode is now done by a custom streaming parameter set via VIDIOC_S_PARM ioctl instead of using a V4L2_BUF_TYPE_PRIVATE buffer (which was removed from V4L2), * General small code cleanups (like using BIT() or ARRAY_SIZE() macros instead of open coding them, code formatting improvements, etc.). Changes from v1: * Only support configuration of cx25840 pins that the cxusb driver is actually using so there is no need for an ugly CX25840_PIN() macro, * Split cxusb changes into two patches: first one implementing digital / analog coexistence in this driver, second one adding the actual implementation of the analog mode, * Fix a warning reported by kbuild test robot. Changes from v2: * Split out ivtv cx25840 platform data zero-initialization to a separate commit, * Add kernel-doc description of struct cx25840_state, * Make sure that all variables used in CX25840_VCONFIG_OPTION() and CX25840_VCONFIG_SET_BIT() macros are their explicit parameters, * Split out some code from cxusb_medion_copy_field() and cxusb_medion_v_complete_work() functions to separate ones to increase their readability, * Generate masks using GENMASK() and BIT() macros in cx25840.h and cxusb.h. Changes from v3: Add SPDX tag to a newly added "cxusb-analog.c" file. Changes from v4: * Make analog support conditional on a new DVB_USB_CXUSB_ANALOG Kconfig option, * Use '//' comments in the header of a newly added "cxusb-analog.c" file, * Don't print errors on memory allocation failures, * Get rid of the driver MODULE_VERSION(), * Small formating fix of a one line. Changes from v5: Rebase onto current media_tree/master. drivers/media/i2c/cx25840/cx25840-core.c | 396 ++- drivers/media/i2c/cx25840/cx25840-core.h | 46 +- drivers/media/i2c/cx25840/cx25840-vbi.c |3 + drivers/media/pci/ivtv/ivtv-i2c.c|1 + drivers/media/tuners/tuner-simple.c |5 +- drivers/media/usb/dvb-usb/Kconfig| 16 +- drivers/media/usb/dvb-usb/Makefile |3 + drivers/media/usb/dvb-usb/cxusb-analog.c | 1914 ++ drivers/media/usb/dvb-usb/cxusb.c| 452 ++- drivers/media/usb/dvb-usb/cxusb.h| 154 +++ drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 20 +- drivers/media/usb/dvb-usb/dvb-usb-init.c | 13 + drivers/media/usb/dvb-usb/dvb-usb.h |8 + include/media/drv-intf/cx25840.h | 74 +- 14 files changed, 3042 insertions(+), 63 deletions(-) create mode 100644 drivers/media/usb/dvb-usb/cxusb-analog.c
[PATCH] x86/speculation: Fill the RSB on context switch also on non-IBPB CPUs
If we run on a CPU that does not have IBPB support RSB entries from one
userspace process can influence 'ret' target prediction in another
userspace process after a context switch.
Since it is unlikely that existing RSB entries from the previous task match
the new task call stack we can use the existing unconditional
RSB-filling-on-context-switch infrastructure to protect against such
userspace-to-userspace attacks.
This patch brings a change in behavior only for the following CPU types:
* Intel pre-Skylake CPUs without updated microcode,
* AMD Family 15h model >60h, Family 17h CPUs without updated microcode.
Other CPU types either already do the RSB filling on context switch for
other reasons or do support IBPB for more complete userspace-to-userspace
protection.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
arch/x86/kernel/cpu/bugs.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index bfca937bdcc3..777bae86e159 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -280,8 +280,11 @@ static void __init spectre_v2_select_mitigation(void)
/*
* If neither SMEP nor PTI are available, there is a risk of
* hitting userspace addresses in the RSB after a context switch
-* from a shallow call stack to a deeper one. To prevent this fill
-* the entire RSB, even when using IBRS.
+* from a shallow call stack to a deeper one.
+* Also, if the CPU does not have IBPB support then one userspace
+* process can influence 'ret' target prediction for another
+* userspace process.
+* To prevent this fill the entire RSB, even when using IBRS.
*
* Skylake era CPUs have a separate issue with *underflow* of the
* RSB, when they will predict 'ret' targets from the generic BTB.
@@ -290,7 +293,8 @@ static void __init spectre_v2_select_mitigation(void)
* switch is required.
*/
if ((!boot_cpu_has(X86_FEATURE_PTI) &&
-!boot_cpu_has(X86_FEATURE_SMEP)) || is_skylake_era()) {
+!boot_cpu_has(X86_FEATURE_SMEP)) ||
+!boot_cpu_has(X86_FEATURE_IBPB) || is_skylake_era()) {
setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
pr_info("Spectre v2 mitigation: Filling RSB on context
switch\n");
}
Re: [PATCH] x86/speculation: Fill the RSB on context switch also on non-IBPB CPUs
On 22.03.2018 00:30, Dave Hansen wrote: > On 03/20/2018 04:17 AM, Maciej S. Szmigiero wrote: >> Since it is unlikely that existing RSB entries from the previous task match >> the new task call stack we can use the existing unconditional >> RSB-filling-on-context-switch infrastructure to protect against such >> userspace-to-userspace attacks. >> >> This patch brings a change in behavior only for the following CPU types: >> * Intel pre-Skylake CPUs without updated microcode, >> * AMD Family 15h model >60h, Family 17h CPUs without updated microcode. >> >> Other CPU types either already do the RSB filling on context switch for >> other reasons or do support IBPB for more complete userspace-to-userspace >> protection. > > I think I misunderstood your reasoning a bit. Let me see if I can > restate the problem a bit. > > IBPB provides provides userspace-to-userspace protection because it > prevents all indirect branch predictions after the barrier from being > controlled by software executed before the barrier. We only use IBPB > for KVM and when processes clear their dumpable flag. > > You're saying that, even if we don't have IBPB, we can do *some* > userspace-to-userspace protection with RSB manipulation. The RSB > manipulation obviously only helps 'RET' instructions and not JMP/CALL, > but it does do *something* useful. > > Is that right? Yes. As far as I understand the issue this should provide a good protection for userspace processes that were recompiled with retpolines as they won't have any indirect jumps and calls. > Do you really want this behavior on all context switches? We don't do > IBPB on all context switches, only the ones where we are switching *to* > a non-dumpable process. > > Do you perhaps want to do RSB manipulation in lieu of IBPB when > switching *to* a non-dumpable process and IBPB is not available? > Is it worth differentiating such processes in this case? IBPB is supposed to be very expensive so certainly it is worthwhile to do it only for high-value processes (=non-dumpable). However, it is unlikely that existing RSB entries from the previous task match the new task call stack anyway. We already do unconditional RSB-filling-on-context-switch in many cases. Maciej
Re: [PATCH] x86/speculation: Fill the RSB on context switch also on non-IBPB CPUs
On 21.03.2018 15:05, Dave Hansen wrote: > On 03/20/2018 04:17 AM, Maciej S. Szmigiero wrote: >> If we run on a CPU that does not have IBPB support RSB entries from one >> userspace process can influence 'ret' target prediction in another >> userspace process after a context switch. >> >> Since it is unlikely that existing RSB entries from the previous task match >> the new task call stack we can use the existing unconditional >> RSB-filling-on-context-switch infrastructure to protect against such >> userspace-to-userspace attacks. >> >> This patch brings a change in behavior only for the following CPU types: >> * Intel pre-Skylake CPUs without updated microcode, > > The assumption thus far (good or bad) is that everything will get a > microcode update. I actually don't know for sure if RSB manipulation is > effective on old microcode before Skylake. I'm pretty sure it has not > been documented publicly. > > How did you decide that this is an effective mitigation? > A RSB overwrite is already being done even on pre-Skylake Intel CPUs on VMEXIT to protect the host from the guest, regardless of the microcode version. But I see that an Intel guidance document published last month about retpolines says that "RET has this [predictable speculative] behavior on all processors (...) microarchitecture codename Broadwell and earlier when updated with the latest microcode". This suggests that updated microcode may be needed for protection anyway on such CPUs - as you say. Such update (hopefully) brings IBPB support, too, so I agree that the change introduced by this patch can be skipped on Intel CPUs. Maciej
Re: [PATCH v4 06/10] x86/microcode/AMD: Check patch size in verify_and_add_patch()
On 22.03.2018 17:11, Borislav Petkov wrote:
> On Fri, Mar 16, 2018 at 12:08:17AM +0100, Maciej S. Szmigiero wrote:
>> @@ -505,7 +505,7 @@ static unsigned int verify_patch_size(u8 family, u32
>> patch_size,
>> break;
>> }
>>
>> -if (patch_size > min_t(u32, size, max_size)) {
>> +if (patch_size > min_t(size_t, size, max_size)) {
>
> So I don't like this conversion to 8-byte-width size_t's. It is not
> necessary. I'm pretty sure we can do fine with signed and unsigned ints.
It is possible to keep verify_patch_size() unmodified (with unsigned int
and u32) but microcode container files >4GB in size then may be rejected,
even if they are technically valid (while a bit unrealistic) on 64-bit
kernels.
Thanks,
Maciej
Re: [PATCH] x86/speculation: Fill the RSB on context switch also on non-IBPB CPUs
On 22.03.2018 16:46, Dave Hansen wrote: > On 03/21/2018 05:09 PM, Maciej S. Szmigiero wrote: >> As far as I understand the issue this should provide a good protection >> for userspace processes that were recompiled with retpolines as they >> won't have any indirect jumps and calls. > > Instead of saying "good protection", let's just say that it could > mitigate attacks that require consumption of attacker-placed RSB entries. All right. >>> Do you perhaps want to do RSB manipulation in lieu of IBPB when >>> switching *to* a non-dumpable process and IBPB is not available? >> >> Is it worth differentiating such processes in this case? >> IBPB is supposed to be very expensive so certainly it is worthwhile >> to do it only for high-value processes (=non-dumpable). >> >> However, it is unlikely that existing RSB entries from the previous >> task match the new task call stack anyway. >> We already do unconditional RSB-filling-on-context-switch in many >> cases. > > I think this case is a bit too obscure and theoretical to complicate the > kernel with it. You need an unmitigated processor, a > userspace-to-userspace attack that manages to satisfy the five "exploit > composition" steps of Spectre/V2[1], and an application that has been > retpoline-mitigated. > > While RSB manipulation is almost certainly less onerous than IBPB, it's > still going to hurt context-switch rates, especially if applied > indiscriminately like this patch does. > > So, I totally agree with your analysis about the theoretical potential > for an issue, I'm just not really convinced the fix is worth it. Yes, Spectre v2 looks really hard to exploit, but this doesn't mean the kernel shouldn't do its best to mitigate it. As I wrote two messages ago, basing on the Intel guidance document you linked above as "[1]" I think that the mitigation introduced by this patch should not be done on Intel CPUs, however, since that document clearly suggests that this may not be enough to cover the issue. And I think we shouldn't give people a false sense of security. Maciej
Re: [PATCH] X.509: unpack RSA signatureValue field from BIT STRING
On 07.03.2018 18:56, Maciej S. Szmigiero wrote:
> On 07.03.2018 16:44, David Howells wrote:
>> Maciej S. Szmigiero <m...@maciej.szmigiero.name> wrote:
>>
>>> + if (!strcmp(ctx->cert->sig->pkey_algo, "rsa")) {
>>
>> I'm going to change this to '== 0' rather than '!'.
>
> No problem.
I cannot find this patch in any tree that I have looked at.
Are you going to pick it up later or am I not looking at the right
place?
Thanks,
Maciej
Re: [PATCH 2/3] crypto: ccp - return an actual key size from RSA max_size callback
On 02.03.2018 17:44, Herbert Xu wrote:
> On Sat, Feb 24, 2018 at 05:03:21PM +0100, Maciej S. Szmigiero wrote:
>> rsa-pkcs1pad uses a value returned from a RSA implementation max_size
>> callback as a size of an input buffer passed to the RSA implementation for
>> encrypt and sign operations.
>>
>> CCP RSA implementation uses a hardware input buffer which size depends only
>> on the current RSA key length, so it should return this key length in
>> the max_size callback, too.
>> This also matches what the kernel software RSA implementation does.
>>
>> Previously, the value returned from this callback was always the maximum
>> RSA key size the CCP hardware supports.
>> This resulted in this huge buffer being passed by rsa-pkcs1pad to CCP even
>> for smaller key sizes and then in a buffer overflow when ccp_run_rsa_cmd()
>> tried to copy this large input buffer into a RSA key length-sized hardware
>> input buffer.
>>
>> Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
>> Fixes: ceeec0afd684 ("crypto: ccp - Add support for RSA on the CCP")
>> Cc: sta...@vger.kernel.org
>
> Patch applied. Thanks.
Thanks.
However, what about the first patch from this series?
Without it, while it no longer should cause a buffer overflow, in-kernel
X.509 certificate verification will still fail with CCP driver loaded
(since CCP RSA implementation has a higher priority than the software
RSA implementation).
Maciej
Re: [PATCH 2/3] crypto: ccp - return an actual key size from RSA max_size callback
On 03.03.2018 00:49, Hook, Gary wrote:
> On 3/2/2018 5:15 PM, Maciej S. Szmigiero wrote:
>> On 02.03.2018 17:44, Herbert Xu wrote:
>>> On Sat, Feb 24, 2018 at 05:03:21PM +0100, Maciej S. Szmigiero wrote:
>>>> rsa-pkcs1pad uses a value returned from a RSA implementation max_size
>>>> callback as a size of an input buffer passed to the RSA implementation for
>>>> encrypt and sign operations.
>>>>
>>>> CCP RSA implementation uses a hardware input buffer which size depends only
>>>> on the current RSA key length, so it should return this key length in
>>>> the max_size callback, too.
>>>> This also matches what the kernel software RSA implementation does.
>>>>
>>>> Previously, the value returned from this callback was always the maximum
>>>> RSA key size the CCP hardware supports.
>>>> This resulted in this huge buffer being passed by rsa-pkcs1pad to CCP even
>>>> for smaller key sizes and then in a buffer overflow when ccp_run_rsa_cmd()
>>>> tried to copy this large input buffer into a RSA key length-sized hardware
>>>> input buffer.
>>>>
>>>> Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
>>>> Fixes: ceeec0afd684 ("crypto: ccp - Add support for RSA on the CCP")
>>>> Cc: sta...@vger.kernel.org
>>>
>>> Patch applied. Thanks.
>>
>> Thanks.
>>
>> However, what about the first patch from this series?
>> Without it, while it no longer should cause a buffer overflow, in-kernel
>> X.509 certificate verification will still fail with CCP driver loaded
>> (since CCP RSA implementation has a higher priority than the software
>> RSA implementation).
>>
>> Maciej
>>
>
>
> I commented on that one here:
> https://marc.info/?l=linux-crypto-vger=151986452422791=2
>
> Effectively a NACK. We are a reviewing a proposed patch right now.
Your earlier comment referred to the third patch from this series.
My message above was about the first one.
Maciej
Re: R8169: Network lockups in 4.18.{8,9,10} (and 4.19 dev)
On 09.10.2018 22:36, Heiner Kallweit wrote:
> On 09.10.2018 16:40, Chris Clayton wrote:
>> Thanks to Maciej and Heiner for their replies.
>>
>> On 09/10/2018 13:32, Maciej S. Szmigiero wrote:
>>> On 07.10.2018 21:36, Chris Clayton wrote:
>>>> Hi again,
>>>>
>>>> I didn't think there was anything in 4.19-rc7 to fix this regression, but
>>>> tried it anyway. I can confirm that the
>>>> regression is still present and my network still fails when, after a
>>>> resume from suspend (to ram or disk), I open my
>>>> browser or my mail client. In both those cases the failure is almost
>>>> immediate - e.g. my home page doesn't get displayed
>>>> in the browser. Pinging one of my ISPs name servers doesn't fail quite so
>>>> quickly but the reported time increases from
>>>> 14-15ms to more than 1000ms.
>>>
>>> You can try comparing chip registers (ethtool -d eth0) in the working
>>> state (before a suspend) and in the broken state (after a resume).
>>> Maybe there will be some obvious in the difference.
>>>
>>> The same goes for the PCI configuration (lspci -d :8168 -vv).
>>>
>> Maciej suggested comparing the output from lspci -vv for the ethernet
>> device. They are identical.
>>
>> Both Maciej and Heiner suggested comparing the output from "ethtool -d" pre
>> and post suspend. Again, they are identical.
>> Heiner specifically suggested looking at the RxConfig. The value of that is
>> 0x0002870e both pre and post suspend.
>>
> Hmm, this is very weird, especially taking into account that in your original
> report you state that removing the call to rtl_init_rxcfg() from
> rtl_hw_start()
> fixes the issue. rtl_init_rxcfg() deals with the RxConfig register only and
> register values seem to be the same before and after resume. So how can the
> chip behave differently?
> So far my best guess is that some chip quirk causes it to accept writes to
> register RxConfig, but to misinterpret or ignore the written value.
> So far your report is the only one (affecting RTL8411), but we don't know
> whether other chip versions are affected too.
Also, it is interesting that even if one removes a call to
rtl_init_rxcfg() from rtl_hw_start() the RxConfig register will still get
written to moments later by rtl_set_rx_mode().
The only chip accesses in the meantime seems to be a write to TxConfig by
rtl_set_tx_config_registers() and then a read of RxConfig plus two writes
to MAR0 earlier in rtl_set_rx_mode().
My proposals are:
1) Try swapping "rtl_init_rxcfg(tp);" and "rtl_set_tx_config_registers(tp);"
in rtl_hw_start().
Maybe the chip does not like sometimes that RxConfig is written before
TxConfig.
2) Check the original value of RxConfig (after a resume) before
rtl_init_rxcfg() overwrites it (compile tested only):
--- r8169.c.ori
+++ r8169.c
@@ -5155,6 +5155,9 @@
/* Initially a 10 us delay. Turned it into a PCI commit. - FR */
RTL_R8(tp, IntrMask);
RTL_W8(tp, ChipCmd, CmdTxEnb | CmdRxEnb);
+
+ pr_notice("RxConfig before init was %.8x\n",
+ (unsigned int)RTL_R32(tp, RxConfig));
rtl_init_rxcfg(tp);
rtl_set_tx_config_registers(tp);
This should be the value that you got when you removed the call to
rtl_init_rxcfg() for testing.
Now, knowing the "right" value you can experiment with what rtl_init_rxcfg()
writes (under the "default:" label for your NIC model).
Hope this helps,
Maciej
Re: R8169: Network lockups in 4.18.{8,9,10} (and 4.19 dev)
On 07.10.2018 21:36, Chris Clayton wrote: > Hi again, > > I didn't think there was anything in 4.19-rc7 to fix this regression, but > tried it anyway. I can confirm that the > regression is still present and my network still fails when, after a resume > from suspend (to ram or disk), I open my > browser or my mail client. In both those cases the failure is almost > immediate - e.g. my home page doesn't get displayed > in the browser. Pinging one of my ISPs name servers doesn't fail quite so > quickly but the reported time increases from > 14-15ms to more than 1000ms. You can try comparing chip registers (ethtool -d eth0) in the working state (before a suspend) and in the broken state (after a resume). Maybe there will be some obvious in the difference. The same goes for the PCI configuration (lspci -d :8168 -vv). > Chris Maciej
Re: R8169: Network lockups in 4.18.{8,9,10} (and 4.19 dev)
On 11.10.2018 00:49, Chris Clayton wrote:
>> Now, knowing the "right" value you can experiment with what rtl_init_rxcfg()
>> writes (under the "default:" label for your NIC model).
>>
>
> This might be more interesting. Through a combination of viewing the output
> from pr_notice() and the output from
> "ethtool -d", I can see RxConfig with the following values
>
> During boot:0x00028700
> Before suspend: 0x0002870e
> During resume: 0x00024000
> Post resume:0x0002870e
>
> As I did with 4.18.10 early on in the process, I removed the call to
> rtl_init_rxcfg() from rtl_hw_start() and rebuilt,
> installed and rebooted. Now I see the following values:
>
> During boot:0x00028700
> Before suspend: 0x0002870e
> During resume: 0x00024000
> Post resume:0x0002400e
>
Now we can finally see some difference...
Besides missing RX128_INT_EN (bit 15 or 0x8000) and RX_DMA_BURST
(bits 8-10 or 0x700) - that rtl_init_rxcfg() would normally set so this
is kind of expected - one can see that the working configuration
post-resume has bit 14 (or 0x4000) set, too.
This bit is described in the driver as RX_MULTI_EN ("8111c only") and is
set by rtl_init_rxcfg() for example for RTL_GIGA_MAC_VER_35.
RTL_GIGA_MAC_VER_35 is described in the driver as being in the same
family as your RTL_GIGA_MAC_VER_38, so can you please try the following
change:
--- r8169.c
+++ r8169.c
@@ -4271,6 +4271,7 @@ static void rtl_init_rxcfg(struct rtl816
case RTL_GIGA_MAC_VER_18 ... RTL_GIGA_MAC_VER_24:
case RTL_GIGA_MAC_VER_34:
case RTL_GIGA_MAC_VER_35:
+ case RTL_GIGA_MAC_VER_38:
RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN |
RX_DMA_BURST);
break;
case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
This will add RX_MULTI_EN also for your chip model (you need to add back
the call to rtl_init_rxcfg() to rtl_hw_start(), naturally).
If this does not help then I would try another values in the above write:
1) RTL_W32(tp, RxConfig, 0x00024000);
2) RTL_W32(tp, RxConfig, 0x4000);
3) RTL_W32(tp, RxConfig, RX_DMA_BURST);
4) RTL_W32(tp, RxConfig, RX128_INT_EN);
> Chris
Maciej
Re: R8169: Network lockups in 4.18.{8,9,10} (and 4.19 dev)
On 11.10.2018 10:24, Chris Clayton wrote:
> On 11/10/2018 01:12, Maciej S. Szmigiero wrote:
>> On 11.10.2018 00:49, Chris Clayton wrote:
>>>> Now, knowing the "right" value you can experiment with what
>>>> rtl_init_rxcfg()
>>>> writes (under the "default:" label for your NIC model).
>>>>
>>>
>>> This might be more interesting. Through a combination of viewing the output
>>> from pr_notice() and the output from
>>> "ethtool -d", I can see RxConfig with the following values
>>>
>>> During boot:0x00028700
>>> Before suspend: 0x0002870e
>>> During resume: 0x00024000
>>> Post resume:0x0002870e
>>>
>>> As I did with 4.18.10 early on in the process, I removed the call to
>>> rtl_init_rxcfg() from rtl_hw_start() and rebuilt,
>>> installed and rebooted. Now I see the following values:
>>>
>>> During boot:0x00028700
>>> Before suspend: 0x0002870e
>>> During resume: 0x00024000
>>> Post resume:0x0002400e
>>>
>>
>> Now we can finally see some difference...
>> Besides missing RX128_INT_EN (bit 15 or 0x8000) and RX_DMA_BURST
>> (bits 8-10 or 0x700) - that rtl_init_rxcfg() would normally set so this
>> is kind of expected - one can see that the working configuration
>> post-resume has bit 14 (or 0x4000) set, too.
>>
>> This bit is described in the driver as RX_MULTI_EN ("8111c only") and is
>> set by rtl_init_rxcfg() for example for RTL_GIGA_MAC_VER_35.
>>
>> RTL_GIGA_MAC_VER_35 is described in the driver as being in the same
>> family as your RTL_GIGA_MAC_VER_38, so can you please try the following
>> change:
>> --- r8169.c
>> +++ r8169.c
>> @@ -4271,6 +4271,7 @@ static void rtl_init_rxcfg(struct rtl816
>> case RTL_GIGA_MAC_VER_18 ... RTL_GIGA_MAC_VER_24:
>> case RTL_GIGA_MAC_VER_34:
>> case RTL_GIGA_MAC_VER_35:
>> +case RTL_GIGA_MAC_VER_38:
>> RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN |
>> RX_DMA_BURST);
>> break;
>> case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
>>
>> This will add RX_MULTI_EN also for your chip model (you need to add back
>> the call to rtl_init_rxcfg() to rtl_hw_start(), naturally).
>>
>
> That's done the trick. With the above change applied, my network runs running
> fine after a suspend/resume cycle and the
> ping times are back in the 14-15ms range.
Nice!
I will submit a patch, it would be great if you could test it and then
add a "Tested-by:" tag.
> Chris
Maciej
[PATCH] r8169: set RX_MULTI_EN bit in RxConfig for 8168F-family chips
It has been reported that since
commit 05212ba8132b42 ("r8169: set RxConfig after tx/rx is enabled for
RTL8169sb/8110sb devices")
at least RTL_GIGA_MAC_VER_38 NICs work erratically after a resume from
suspend.
The problem has been traced to a missing RX_MULTI_EN bit in the RxConfig
register.
We already set this bit for RTL_GIGA_MAC_VER_35 NICs of the same 8168F
chip family so let's do it also for its other siblings: RTL_GIGA_MAC_VER_36
and RTL_GIGA_MAC_VER_38.
Curiously, the NIC seems to work fine after a system boot without having
this bit set as long as the system isn't suspended and resumed.
Fixes: 05212ba8132b42 ("r8169: set RxConfig after tx/rx is enabled for
RTL8169sb/8110sb devices")
Reported-by: Chris Clayton
Signed-off-by: Maciej S. Szmigiero
---
drivers/net/ethernet/realtek/r8169.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 7d3f671e1bb3..b68e32186d67 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4269,8 +4269,8 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
RTL_W32(tp, RxConfig, RX_FIFO_THRESH | RX_DMA_BURST);
break;
case RTL_GIGA_MAC_VER_18 ... RTL_GIGA_MAC_VER_24:
- case RTL_GIGA_MAC_VER_34:
- case RTL_GIGA_MAC_VER_35:
+ case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
+ case RTL_GIGA_MAC_VER_38:
RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN |
RX_DMA_BURST);
break;
case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
--
2.17.0
Re: R8169: Network lockups in 4.18.{8,9,10} (and 4.19 dev)
Hi, > Hi, > > I upgraded my kernel to 4.18.10 recently and have since been experiencing > network problems after resuming from a > suspend to RAM or disk. I previously had 4.18.6 and that was OK. > > The pattern of the problem is that when I first boot, the network is fine. > But, after resume from suspend I find that > the time taken for a ping of one of my ISP's nameservers increases from > 14-15ms to more than 1000ms. Moreover, when I > open a browser (chromium or firefox), it fails to retrieve my home page > (https://www.google.co.uk) and pings of the > nameserver fail with the message "Destination Host Unreachable". Often, I can > revive the network by stopping it with > /sbin/if(down,up} but sometimes it is necessary to also remove the r8169 > module and load it again. Please have a look at the following thread: https://lkml.org/lkml/2018/9/25/1118 Maciej
Re: kernel 4.18.5 Realtek 8111G network adapter stops responding under high system load
Hi,
On 18.09.2018 12:23, David Arendt wrote:
> Hi,
>
> Today I had the network adapter problems again.
> So the patch doesn't seem to change anything regarding this problem.
> This week my time is unfortunately very limited, but I will try to
> find some time next weekend to look a bit more into the issue.
If the problem is caused by missing TXCFG_AUTO_FIFO bit in TxConfig,
as the register difference would suggest, then you can try applying
the following patch (hack) on top of 4.18.8 that is already patched
with commit f74dd480cf4e:
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5043,7 +5043,8 @@
{
/* Set DMA burst size and Interframe Gap Time */
RTL_W32(tp, TxConfig, (TX_DMA_BURST << TxDMAShift) |
- (InterFrameGap << TxInterFrameGapShift));
+ (InterFrameGap << TxInterFrameGapShift)
+ | TXCFG_AUTO_FIFO);
}
static void rtl_set_rx_max_size(struct rtl8169_private *tp)
This hack will probably only work properly on RTL_GIGA_MAC_VER_40 or
later NICs.
Before running any tests please verify with "ethtool -d enp3s0" that
TxConfig register now contains 0x4f000f80, as it did in the old,
working driver version.
If this does not help then a bisection will most likely be needed.
> Thanks in advance,
> David Arendt
Maciej
[PATCH v3 0/9] x86/microcode/AMD: check microcode file sanity before loading it
Currently, it is very easy to make the AMD microcode update driver crash
or spin on a malformed microcode container file since it does very little
consistency checking on data loaded from such file.
This series introduces various checks, mostly on length-type fields,
so all corrupted microcode container files are (hopefully) correctly
rejected instead.
This largely matches what the Intel microcode update driver already does.
It also tries to make the behavior consistent between the early and late
loaders.
Please note that this isn't about verifying the actual microcode update,
that is, the blob that gets sent to the CPU as the new microcode.
Such verification is (hopefully) done by the CPU itself.
It is about verifying a driver-specific container file that includes
many microcode updates for different CPUs of a particular CPU family,
along with metadata that helps the driver select the right microcode
update to actually send to the CPU.
There are purposely-corrupted test files available at
https://pastebin.com/XkKUSmMp
One has to enable KASAN in the kernel config and rename a particular
test file to name appropriate to the running CPU family to test its
loading.
Changes from v1: Capitalize a comment, rename 'eqsize' and 'bufsize'
to 'eq_size' and 'buf_size', respectively, attach a comment about
checking the equivalence table header to its first size check, rename
'equiv{_cpu,}_table_size' to 'equiv{_cpu,}_table_entries'.
Changes from v2: Split the patch into separate commits, remove explicit
CPU equivalence table size limit, make install_equiv_cpu_table() return
a size_t instead of a (signed) int so no overflow can occur there,
automatically compute the PATCH_MAX_SIZE macro and use it for checking
a patch size, make the late loader behavior with respect to late parse
failures consistent with what the early loader does.
arch/x86/include/asm/microcode_amd.h | 2 -
arch/x86/kernel/cpu/microcode/amd.c | 164 ---
2 files changed, 114 insertions(+), 52 deletions(-)
[PATCH v3 4/9] x86/microcode/AMD: automatically compute the PATCH_MAX_SIZE macro
The PATCH_MAX_SIZE macro should contain the maximum of all family patch
sizes, computed automatically so that future changes there don't cause an
inconsistency.
Unfortunately, we can't use a standard max{,3} macros for this since they
only work inside a function (they use a compound statement as an
expression) and we have a static array using this macro value as its
length.
This macro is specific to amd.c file so let's move it there (the max sizes
for families are in this file already).
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/include/asm/microcode_amd.h | 2 --
arch/x86/kernel/cpu/microcode/amd.c | 22 --
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/microcode_amd.h
b/arch/x86/include/asm/microcode_amd.h
index 209492849566..365bfa85a06b 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -41,8 +41,6 @@ struct microcode_amd {
unsigned intmpb[0];
};
-#define PATCH_MAX_SIZE PAGE_SIZE
-
#ifdef CONFIG_MICROCODE_AMD
extern void __init load_ucode_amd_bsp(unsigned int family);
extern void load_ucode_amd_ap(unsigned int family);
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index d20c327c960b..b9f6c06bdc16 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -40,6 +40,22 @@
static struct equiv_cpu_entry *equiv_cpu_table;
+/* Maximum patch size for a particular family */
+#define F1XH_MPB_MAX_SIZE 2048
+#define F14H_MPB_MAX_SIZE 1824
+#define F15H_MPB_MAX_SIZE 4096
+#define F16H_MPB_MAX_SIZE 3458
+#define F17H_MPB_MAX_SIZE 3200
+
+/* Can't use a standard max{,3} since they only work inside a function */
+#define SIMPLE_MAX(x, y) ((x) > (y) ? (x) : (y))
+#define SIMPLE_MAX3(x, y, z) SIMPLE_MAX(SIMPLE_MAX(x, y), z)
+
+/* Maximum of all the above families */
+#define PATCH_MAX_SIZE SIMPLE_MAX3(F1XH_MPB_MAX_SIZE, F14H_MPB_MAX_SIZE, \
+ SIMPLE_MAX3(F15H_MPB_MAX_SIZE, F16H_MPB_MAX_SIZE, \
+ F17H_MPB_MAX_SIZE))
+
/*
* This points to the current valid container of microcode patches which we
will
* save from the initrd/builtin before jettisoning its contents. @mc is the
@@ -473,12 +489,6 @@ static unsigned int verify_patch_size(u8 family, u32
patch_size,
{
u32 max_size;
-#define F1XH_MPB_MAX_SIZE 2048
-#define F14H_MPB_MAX_SIZE 1824
-#define F15H_MPB_MAX_SIZE 4096
-#define F16H_MPB_MAX_SIZE 3458
-#define F17H_MPB_MAX_SIZE 3200
-
switch (family) {
case 0x14:
max_size = F14H_MPB_MAX_SIZE;
[PATCH v3 5/9] x86/microcode/AMD: check patch size in verify_and_add_patch()
Now that we have the PATCH_MAX_SIZE macro correctly computed we can verify
properly the indicated size of a patch in a microcode container file and
whether this file is actually large enough to contain it in the late loader
verify_and_add_patch() function.
The early loader already does the PATCH_MAX_SIZE check in parse_container()
function.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index b9f6c06bdc16..0f78200f2f6c 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -485,7 +485,7 @@ static int collect_cpu_info_amd(int cpu, struct
cpu_signature *csig)
}
static unsigned int verify_patch_size(u8 family, u32 patch_size,
- unsigned int size)
+ size_t size)
{
u32 max_size;
@@ -507,7 +507,7 @@ static unsigned int verify_patch_size(u8 family, u32
patch_size,
break;
}
- if (patch_size > min_t(u32, size, max_size)) {
+ if (patch_size > min_t(size_t, size, max_size)) {
pr_err("patch size mismatch\n");
return 0;
}
@@ -616,7 +616,7 @@ static void cleanup(void)
* driver cannot continue functioning normally. In such cases, we tear
* down everything we've used up so far and exit.
*/
-static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
+static int verify_and_add_patch(u8 family, u8 *fw, size_t leftover)
{
struct microcode_header_amd *mc_hdr;
struct ucode_patch *patch;
@@ -624,7 +624,15 @@ static int verify_and_add_patch(u8 family, u8 *fw,
unsigned int leftover)
u32 proc_fam;
u16 proc_id;
+ if (leftover < SECTION_HDR_SIZE + sizeof(*mc_hdr))
+ return leftover;
+
patch_size = *(u32 *)(fw + 4);
+ if (patch_size > PATCH_MAX_SIZE) {
+ pr_err("patch size %u too large\n", patch_size);
+ return -EINVAL;
+ }
+
crnt_size = patch_size + SECTION_HDR_SIZE;
mc_hdr = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
proc_id = mc_hdr->processor_rev_id;
[PATCH v3 2/9] x86/microcode/AMD: check whether the equivalence table fits in the file
Before loading a CPU equivalence table from a microcode container file we
need to verify whether this file is actually large enough to contain the
table of a size indicated in this file.
If it is not, there is no point of continuing with loading it since
microcode patches are located after the equivalence table anyway.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 54 ++---
1 file changed, 38 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index ffe0d0ce57fc..ac06e2819f26 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -80,20 +80,29 @@ static u16 find_equiv_id(struct equiv_cpu_entry
*equiv_table, u32 sig)
* Returns the amount of bytes consumed while scanning. @desc contains all the
* data we're going to use in later stages of the application.
*/
-static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
+static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
{
struct equiv_cpu_entry *eq;
- ssize_t orig_size = size;
+ size_t orig_size = size;
u32 *hdr = (u32 *)ucode;
+ size_t eq_size;
u16 eq_id;
u8 *buf;
/* Am I looking at an equivalence table header? */
+ if (size < CONTAINER_HDR_SZ)
+ return 0;
+
if (hdr[0] != UCODE_MAGIC ||
hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
hdr[2] == 0)
return CONTAINER_HDR_SZ;
+ eq_size = hdr[2];
+ if (eq_size < sizeof(*eq) ||
+ size - CONTAINER_HDR_SZ < eq_size)
+ return 0;
+
buf = ucode;
eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
@@ -101,8 +110,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
/* Find the equivalence ID of our CPU in this table: */
eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
- buf += hdr[2] + CONTAINER_HDR_SZ;
- size -= hdr[2] + CONTAINER_HDR_SZ;
+ buf += eq_size + CONTAINER_HDR_SZ;
+ size -= eq_size + CONTAINER_HDR_SZ;
/*
* Scan through the rest of the container to find where it ends. We do
@@ -159,15 +168,13 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
*/
static void scan_containers(u8 *ucode, size_t size, struct cont_desc *desc)
{
- ssize_t rem = size;
-
- while (rem >= 0) {
- ssize_t s = parse_container(ucode, rem, desc);
+ while (size > 0) {
+ size_t s = parse_container(ucode, size, desc);
if (!s)
return;
ucode += s;
- rem -= s;
+ size -= s;
}
}
@@ -540,15 +547,30 @@ static enum ucode_state apply_microcode_amd(int cpu)
return UCODE_UPDATED;
}
-static int install_equiv_cpu_table(const u8 *buf)
+static int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
{
unsigned int *ibuf = (unsigned int *)buf;
- unsigned int type = ibuf[1];
- unsigned int size = ibuf[2];
+ unsigned int type, size;
+
+ if (buf_size < CONTAINER_HDR_SZ) {
+ pr_err("no container header\n");
+ return -EINVAL;
+ }
+
+ type = ibuf[1];
+ if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
+ pr_err("invalid type field in container file section header\n");
+ return -EINVAL;
+ }
+
+ size = ibuf[2];
+ if (size < sizeof(struct equiv_cpu_entry)) {
+ pr_err("equivalent CPU table too short\n");
+ return -EINVAL;
+ }
- if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
- pr_err("empty section/"
- "invalid type field in container file section header\n");
+ if (buf_size - CONTAINER_HDR_SZ < size) {
+ pr_err("equivalent CPU table truncated\n");
return -EINVAL;
}
@@ -655,7 +677,7 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
int crnt_size = 0;
int offset;
- offset = install_equiv_cpu_table(data);
+ offset = install_equiv_cpu_table(data, size);
if (offset < 0) {
pr_err("failed to create equivalent cpu table\n");
return ret;
[PATCH v3 6/9] x86/microcode/AMD: verify patch section type for every such section
We should check whether the patch section currently being processed is
actually a patch section for each of them (not just the first one) in the
late loader verify_and_add_patch() function, just like the early loader
already does in parse_container() function.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 0f78200f2f6c..3ad23e72c2b0 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -627,6 +627,11 @@ static int verify_and_add_patch(u8 family, u8 *fw, size_t
leftover)
if (leftover < SECTION_HDR_SIZE + sizeof(*mc_hdr))
return leftover;
+ if (*(u32 *)fw != UCODE_UCODE_TYPE) {
+ pr_err("invalid type field in container file section header\n");
+ return -EINVAL;
+ }
+
patch_size = *(u32 *)(fw + 4);
if (patch_size > PATCH_MAX_SIZE) {
pr_err("patch size %u too large\n", patch_size);
@@ -704,12 +709,6 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
fw += offset;
leftover = size - offset;
- if (*(u32 *)fw != UCODE_UCODE_TYPE) {
- pr_err("invalid type field in container file section header\n");
- free_equiv_cpu_table();
- return ret;
- }
-
while (leftover) {
crnt_size = verify_and_add_patch(family, fw, leftover);
if (crnt_size < 0)
[PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int
The maximum possible value returned by install_equiv_cpu_table() is
UINT_MAX + CONTAINER_HDR_SZ (on a 64-bit kernel).
This is more than (signed) int type currently returned by this function can
hold so this function will need to return a size_t instead.
The individual (negative) error codes returned by this function are of no
use anyway, since they are all normalized to UCODE_ERROR by its caller
(__load_microcode_amd()).
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index ac06e2819f26..d20c327c960b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -547,37 +547,38 @@ static enum ucode_state apply_microcode_amd(int cpu)
return UCODE_UPDATED;
}
-static int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
+static size_t install_equiv_cpu_table(const u8 *buf, size_t buf_size)
{
unsigned int *ibuf = (unsigned int *)buf;
- unsigned int type, size;
+ unsigned int type;
+ size_t size;
if (buf_size < CONTAINER_HDR_SZ) {
pr_err("no container header\n");
- return -EINVAL;
+ return 0;
}
type = ibuf[1];
if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
pr_err("invalid type field in container file section header\n");
- return -EINVAL;
+ return 0;
}
size = ibuf[2];
if (size < sizeof(struct equiv_cpu_entry)) {
pr_err("equivalent CPU table too short\n");
- return -EINVAL;
+ return 0;
}
if (buf_size - CONTAINER_HDR_SZ < size) {
pr_err("equivalent CPU table truncated\n");
- return -EINVAL;
+ return 0;
}
equiv_cpu_table = vmalloc(size);
if (!equiv_cpu_table) {
pr_err("failed to allocate equivalent CPU table\n");
- return -ENOMEM;
+ return 0;
}
memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
@@ -672,13 +673,13 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
size_t size)
{
enum ucode_state ret = UCODE_ERROR;
- unsigned int leftover;
+ size_t leftover;
u8 *fw = (u8 *)data;
int crnt_size = 0;
- int offset;
+ size_t offset;
offset = install_equiv_cpu_table(data, size);
- if (offset < 0) {
+ if (!offset) {
pr_err("failed to create equivalent cpu table\n");
return ret;
}
[PATCH v3 1/9] x86/microcode/AMD: subtract SECTION_HDR_SIZE from file leftover length
verify_patch_size() function verifies whether the microcode container file
remaining size is large enough to contain a patch of the indicated size.
However, the section header length is not included in this indicated size
but it is present in the leftover file length so it should be subtracted
from the leftover file length before passing this value to
verify_patch_size().
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index a998e1a7d46f..ffe0d0ce57fc 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -613,7 +613,8 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned
int leftover)
return crnt_size;
}
- ret = verify_patch_size(family, patch_size, leftover);
+ ret = verify_patch_size(family, patch_size,
+ leftover - SECTION_HDR_SIZE);
if (!ret) {
pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
return crnt_size;
[PATCH v3 7/9] x86/microcode/AMD: check microcode container file size before accessing it
The early loader parse_container() function should check whether the
microcode container file is actually large enough to contain the patch of
an indicated size, just like the late loader does.
Also, the request_microcode_amd() function should check whether the
container file is actually large enough to contain the header magic value.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 3ad23e72c2b0..63bd1a63f98a 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -137,6 +137,9 @@ static size_t parse_container(u8 *ucode, size_t size,
struct cont_desc *desc)
struct microcode_amd *mc;
u32 patch_size;
+ if (size < SECTION_HDR_SIZE)
+ break;
+
hdr = (u32 *)buf;
if (hdr[0] != UCODE_UCODE_TYPE)
@@ -151,6 +154,10 @@ static size_t parse_container(u8 *ucode, size_t size,
struct cont_desc *desc)
buf += SECTION_HDR_SIZE;
size -= SECTION_HDR_SIZE;
+ if (size < sizeof(*mc) ||
+ size < patch_size)
+ break;
+
mc = (struct microcode_amd *)buf;
if (eq_id == mc->hdr.processor_rev_id) {
desc->psize = patch_size;
@@ -786,6 +793,10 @@ static enum ucode_state request_microcode_amd(int cpu,
struct device *device,
}
ret = UCODE_ERROR;
+ if (fw->size < sizeof(u32)) {
+ pr_err("microcode container far too short\n");
+ goto fw_release;
+ }
if (*(u32 *)fw->data != UCODE_MAGIC) {
pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
goto fw_release;
[PATCH v3 8/9] x86/microcode/AMD: check the equivalence table size when scanning it
Currently, the code scanning the CPU equivalence table read from a
microcode container file assumes that it actually contains a terminating
zero entry.
Let's check also the size of this table to make sure that we don't read
past it in case it actually doesn't.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 32 +---
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 63bd1a63f98a..78e698fcd3ce 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -39,6 +39,7 @@
#include
static struct equiv_cpu_entry *equiv_cpu_table;
+static unsigned int equiv_cpu_table_entries;
/* Maximum patch size for a particular family */
#define F1XH_MPB_MAX_SIZE 2048
@@ -79,12 +80,18 @@ static u8 amd_ucode_patch[PATCH_MAX_SIZE];
static const char
ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";
-static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
+static u16 find_equiv_id(const struct equiv_cpu_entry *equiv_table,
+unsigned int equiv_table_entries, u32 sig)
{
- for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
- if (sig == equiv_table->installed_cpu)
- return equiv_table->equiv_cpu;
- }
+ unsigned int i;
+
+ if (!equiv_table)
+ return 0;
+
+ for (i = 0; i < equiv_table_entries && equiv_table[i].installed_cpu;
+i++)
+ if (sig == equiv_table[i].installed_cpu)
+ return equiv_table[i].equiv_cpu;
return 0;
}
@@ -124,7 +131,7 @@ static size_t parse_container(u8 *ucode, size_t size,
struct cont_desc *desc)
eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
/* Find the equivalence ID of our CPU in this table: */
- eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
+ eq_id = find_equiv_id(eq, eq_size / sizeof(*eq), desc->cpuid_1_eax);
buf += eq_size + CONTAINER_HDR_SZ;
size -= eq_size + CONTAINER_HDR_SZ;
@@ -394,20 +401,21 @@ void reload_ucode_amd(void)
static u16 __find_equiv_id(unsigned int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- return find_equiv_id(equiv_cpu_table, uci->cpu_sig.sig);
+ return find_equiv_id(equiv_cpu_table, equiv_cpu_table_entries,
+uci->cpu_sig.sig);
}
static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
{
- int i = 0;
+ unsigned int i;
BUG_ON(!equiv_cpu_table);
- while (equiv_cpu_table[i].equiv_cpu != 0) {
+ for (i = 0; i < equiv_cpu_table_entries &&
+equiv_cpu_table[i].equiv_cpu != 0; i++)
if (equiv_cpu == equiv_cpu_table[i].equiv_cpu)
return equiv_cpu_table[i].installed_cpu;
- i++;
- }
+
return 0;
}
@@ -599,6 +607,7 @@ static size_t install_equiv_cpu_table(const u8 *buf, size_t
buf_size)
}
memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
+ equiv_cpu_table_entries = size / sizeof(struct equiv_cpu_entry);
/* add header length */
return size + CONTAINER_HDR_SZ;
@@ -608,6 +617,7 @@ static void free_equiv_cpu_table(void)
{
vfree(equiv_cpu_table);
equiv_cpu_table = NULL;
+ equiv_cpu_table_entries = 0;
}
static void cleanup(void)
[PATCH v3 9/9] x86/microcode/AMD: be more tolerant of late parse failures in late loader
The early loader just ends its microcode container file processing when it
is unable to parse some patch section, but keeps the already read patches
from this file for their eventual application.
We can do the same in the late loader - we'll just return an error if we
are unable to parse any patches.
Note that we already do silently skip patches in the late loader for
smaller issues like lack of an equivalence table entry, family-size
mismatch or an unsupported chipset match type.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 78e698fcd3ce..a098780b0847 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -729,13 +729,15 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
while (leftover) {
crnt_size = verify_and_add_patch(family, fw, leftover);
if (crnt_size < 0)
- return ret;
+ break;
fw += crnt_size;
leftover -= crnt_size;
+
+ ret = UCODE_OK;
}
- return UCODE_OK;
+ return ret;
}
static enum ucode_state
Re: [PATCH v3 2/9] x86/microcode/AMD: check whether the equivalence table fits in the file
On 14.03.2018 18:04, Borislav Petkov wrote:
> On Tue, Mar 13, 2018 at 10:06:23PM +0100, Maciej S. Szmigiero wrote:
(..)
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> @@ -80,20 +80,29 @@ static u16 find_equiv_id(struct equiv_cpu_entry
>> *equiv_table, u32 sig)
>> * Returns the amount of bytes consumed while scanning. @desc contains all
>> the
>> * data we're going to use in later stages of the application.
>> */
>> -static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc
>> *desc)
>> +static size_t parse_container(u8 *ucode, size_t size, struct cont_desc
>> *desc)
>> {
>> struct equiv_cpu_entry *eq;
>> -ssize_t orig_size = size;
>> +size_t orig_size = size;
>> u32 *hdr = (u32 *)ucode;
>> +size_t eq_size;
>> u16 eq_id;
>> u8 *buf;
>>
>> /* Am I looking at an equivalence table header? */
>
> That comment becomes wrong when you add this check here.
It was moved there because on the first (monolithic) iteration of this
change there was a review comment that it better belongs here.
No problem to move it back, however.
>> +if (size < CONTAINER_HDR_SZ)
>> +return 0;
>> +
>> if (hdr[0] != UCODE_MAGIC ||
>> hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
>> hdr[2] == 0)
>> return CONTAINER_HDR_SZ;
>>
>> +eq_size = hdr[2];
>
> If we're going to have special local vars for the container header, then
> do it right:
>
> cont_magic = hdr[0];
> cont_type = hdr[1];
> equiv_tbl_len = hdr[2];
>
> and then use those from now on.
Will do.
>> +if (eq_size < sizeof(*eq) ||
>> +size - CONTAINER_HDR_SZ < eq_size)
>> +return 0;
>
> I think you want
>
> if (size < eqiv_tbl_len + CONTAINER_HDR_SZ)
> return size;
>
> here to skip over the next, hopefully not truncated container.
'size' here is the length of the whole CPIO blob containing all
containers combined (well, the remaining part of it).
If we skip over 'size' bytes we'll have nothing left to parse.
>> @@ -159,15 +168,13 @@ static ssize_t parse_container(u8 *ucode, ssize_t
>> size, struct cont_desc *desc)
>> */
>> static void scan_containers(u8 *ucode, size_t size, struct cont_desc *desc)
>> {
>> -ssize_t rem = size;
>> -
>> -while (rem >= 0) {
>> -ssize_t s = parse_container(ucode, rem, desc);
>> +while (size > 0) {
>> +size_t s = parse_container(ucode, size, desc);
>> if (!s)
>> return;
>>
>> ucode += s;
>> -rem -= s;
>> +size -= s;
>> }
>> }
>>
>
> All changes upto here need to be a separate patch.
> install_equiv_cpu_table() changes below are the second patch.
OK, will split this into two patches.
>> @@ -540,15 +547,30 @@ static enum ucode_state apply_microcode_amd(int cpu)
>> return UCODE_UPDATED;
>> }
>>
>> -static int install_equiv_cpu_table(const u8 *buf)
>> +static int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
>> {
>> unsigned int *ibuf = (unsigned int *)buf;
>> -unsigned int type = ibuf[1];
>> -unsigned int size = ibuf[2];
>> +unsigned int type, size;
>
> unsigned int type, equiv_tbl_len;
Will do.
>> +
>> +if (buf_size < CONTAINER_HDR_SZ) {
>
><= is ok too.
Will do.
>> +pr_err("no container header\n");
>
> More descriptive error messages:
>
> "Truncated microcode container header.\n"
Will do.
>> +return -EINVAL;
>> +}
>> +
>> +type = ibuf[1];
>> +if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
>> +pr_err("invalid type field in container file section header\n");
>
> "Wrong microcode container equivalence table type:
> %d.\n"
Will do.
>> +return -EINVAL;
>> +}
>> +
>> +size = ibuf[2];
>> +if (size < sizeof(struct equiv_cpu_entry)) {
>> +pr_err("equivalent CPU table too short\n");
>
> "Truncated equivalence table.\n"
Will do.
>> +return -EINVAL;
>> +}
>>
>> -if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
>> -pr_err("empty section/"
>> - "invalid type field in container file section header\n");
>> +if (buf_size - CONTAINER_HDR_SZ < size) {
>> +pr_err("equivalent CPU table truncated\n");
>
> Combine that test with the above one and use the same error message.
Will do.
> Thx.
>
Thanks,
Maciej
Re: [PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int
On 14.03.2018 18:58, Borislav Petkov wrote: > On Tue, Mar 13, 2018 at 10:06:34PM +0100, Maciej S. Szmigiero wrote: >> The maximum possible value returned by install_equiv_cpu_table() is >> UINT_MAX + CONTAINER_HDR_SZ (on a 64-bit kernel). >> This is more than (signed) int type currently returned by this function can >> hold so this function will need to return a size_t instead. > > I'm trying to parse this but I'm not really sure. > > All I know is: > > unsigned int size = ibuf[2]; > > and that is really a 4-byte unsigned quantity so anything less is an > arbitrary limitation. There is no limit on CPU equivalence table length in this patch series like it was in the previous version. The maximum possible value returned by install_equiv_cpu_table() of UINT_MAX + CONTAINER_HDR_SZ comes from the maximum value of this 'size' variable (that is UINT_MAX) plus the header length of CONTAINER_HDR_SZ. This won't fit in 'int' type, hence this patch. That's because this functions tells its caller how much bytes to skip from the beginning of a microcode container file to the first patch section contained in it. Maciej
Re: [PATCH v3 4/9] x86/microcode/AMD: automatically compute the PATCH_MAX_SIZE macro
On 14.03.2018 19:02, Borislav Petkov wrote:
> On Tue, Mar 13, 2018 at 10:06:48PM +0100, Maciej S. Szmigiero wrote:
>> +/* Maximum of all the above families */
>> +#define PATCH_MAX_SIZE SIMPLE_MAX3(F1XH_MPB_MAX_SIZE, F14H_MPB_MAX_SIZE, \
>
> Nope, it should be
>
> #define PATCH_MAX_SIZE (max_t(unsigned int, FXXH...
Unfortunately, this does not work:
> ./include/linux/kernel.h:806:41: error: braced-group within expression
> allowed only inside a function
> #define __max(t1, t2, max1, max2, x, y) ({ \
That's because we have a static array containing the chosen microcode
patch for the current CPU (amd_ucode_patch) using this macro value as its
length.
Comments in the code say we can't use vmalloc() during early microcode
load so we can't allocate this array dynamically.
And if we hardcode its length we don't get the benefits of automatically
computing this length as maximum of all family patch sizes (as it should
be).
Maciej
Re: [PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int
On 15.03.2018 00:58, Borislav Petkov wrote: > On Thu, Mar 15, 2018 at 12:46:05AM +0100, Maciej S. Szmigiero wrote: >> The maximum possible value returned by install_equiv_cpu_table() of >> UINT_MAX + CONTAINER_HDR_SZ comes from the maximum value of this 'size' >> variable (that is UINT_MAX) plus the header length of CONTAINER_HDR_SZ. >> This won't fit in 'int' type, hence this patch. > > So make it fit by returning an unsigned int. > This can be done if this function is modified to return only the CPU equivalence table length (without the container header length), leaving its single caller the job of adding the container header length to skip to the fist patch section. Otherwise we introduce a equivalence table length limit of UINT_MAX - CONTAINER_HDR_SZ, as anything more will overflow an unsigned int variable on a 64-bit kernel (on 32-bit this will be caught by the equivalence table truncation check). Maciej
Re: [PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int
On 15.03.2018 01:56, Borislav Petkov wrote: > On Thu, Mar 15, 2018 at 01:13:07AM +0100, Maciej S. Szmigiero wrote: >> This can be done if this function is modified to return only the CPU >> equivalence table length (without the container header length), leaving >> its single caller the job of adding the container header length to skip >> to the fist patch section. > > Sure, it leaves the function to deal with the equiv table length only > and the caller then adds the header length. Which is actually cleaner. > OK, will do then. Maciej
[PATCH v6 0/6] [media] Add analog mode support for Medion MD95700
This series adds support for analog part of Medion 95700 in the cxusb driver. What works: * Video capture at various sizes with sequential fields, * Input switching (TV Tuner, Composite, S-Video), * TV and radio tuning, * Video standard switching and auto detection, * Radio mode switching (stereo / mono), * Unplugging while capturing, * DVB / analog coexistence, * Raw BT.656 stream support. What does not work yet: * Audio, * VBI, * Picture controls. This series (as a one patch) was submitted for inclusion few years ago, then waited few months in a patch queue. Unfortunately, by the time it was supposed to be merged there were enough changes in media that it was no longer mergable. I thought at that time that I will be able to rebase and retest it soon but unfortunately up till now I was never able to find enough time to do so. Also, with the passing of time the implementation diverged more and more from the current kernel code, necessitating even more reworking. That last iteration can be found here: https://patchwork.linuxtv.org/patch/8048/ Since that version there had been the following changes: * Adaptation to changes in V4L2 / DVB core, * Radio device was added, with a possibility to tune to a FM radio station and switch between stereo and mono modes (tested by taping audio signal directly at tuner output pin), * DVB / analog coexistence was improved - resolved a few cases where DVB core would switch off power or reset the tuner when the device was still being used but in the analog mode, * Fixed issues reported by v4l2-compliance, * Switching to raw BT.656 mode is now done by a custom streaming parameter set via VIDIOC_S_PARM ioctl instead of using a V4L2_BUF_TYPE_PRIVATE buffer (which was removed from V4L2), * General small code cleanups (like using BIT() or ARRAY_SIZE() macros instead of open coding them, code formatting improvements, etc.). Changes from v1: * Only support configuration of cx25840 pins that the cxusb driver is actually using so there is no need for an ugly CX25840_PIN() macro, * Split cxusb changes into two patches: first one implementing digital / analog coexistence in this driver, second one adding the actual implementation of the analog mode, * Fix a warning reported by kbuild test robot. Changes from v2: * Split out ivtv cx25840 platform data zero-initialization to a separate commit, * Add kernel-doc description of struct cx25840_state, * Make sure that all variables used in CX25840_VCONFIG_OPTION() and CX25840_VCONFIG_SET_BIT() macros are their explicit parameters, * Split out some code from cxusb_medion_copy_field() and cxusb_medion_v_complete_work() functions to separate ones to increase their readability, * Generate masks using GENMASK() and BIT() macros in cx25840.h and cxusb.h. Changes from v3: Add SPDX tag to a newly added "cxusb-analog.c" file. Changes from v4: * Make analog support conditional on a new DVB_USB_CXUSB_ANALOG Kconfig option, * Use '//' comments in the header of a newly added "cxusb-analog.c" file, * Don't print errors on memory allocation failures, * Get rid of the driver MODULE_VERSION(), * Small formating fix of a one line. Changes from v5: Rebase onto current media_tree/master. drivers/media/i2c/cx25840/cx25840-core.c | 396 ++- drivers/media/i2c/cx25840/cx25840-core.h | 46 +- drivers/media/i2c/cx25840/cx25840-vbi.c |3 + drivers/media/pci/ivtv/ivtv-i2c.c|1 + drivers/media/tuners/tuner-simple.c |5 +- drivers/media/usb/dvb-usb/Kconfig| 16 +- drivers/media/usb/dvb-usb/Makefile |3 + drivers/media/usb/dvb-usb/cxusb-analog.c | 1914 ++ drivers/media/usb/dvb-usb/cxusb.c| 452 ++- drivers/media/usb/dvb-usb/cxusb.h| 154 +++ drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 20 +- drivers/media/usb/dvb-usb/dvb-usb-init.c | 13 + drivers/media/usb/dvb-usb/dvb-usb.h |8 + include/media/drv-intf/cx25840.h | 74 +- 14 files changed, 3042 insertions(+), 63 deletions(-) create mode 100644 drivers/media/usb/dvb-usb/cxusb-analog.c
[PATCH v6 5/6] [media] cxusb: implement Medion MD95700 digital / analog coexistence
This patch prepares cxusb driver for supporting the analog part of
Medion 95700 (previously only the digital - DVB - mode was supported).
Specifically, it adds support for:
* switching the device between analog and digital modes of operation,
* enforcing that only one mode is active at the same time due to hardware
limitations.
Actual implementation of the analog mode will be provided by the next
commit.
Signed-off-by: Maciej S. Szmigiero
---
drivers/media/usb/dvb-usb/cxusb.c| 450 +++
drivers/media/usb/dvb-usb/cxusb.h| 48
drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 20 +-
drivers/media/usb/dvb-usb/dvb-usb-init.c | 13 +
drivers/media/usb/dvb-usb/dvb-usb.h | 8 +
5 files changed, 486 insertions(+), 53 deletions(-)
diff --git a/drivers/media/usb/dvb-usb/cxusb.c
b/drivers/media/usb/dvb-usb/cxusb.c
index 387a074ea6ec..11df020b7de3 100644
--- a/drivers/media/usb/dvb-usb/cxusb.c
+++ b/drivers/media/usb/dvb-usb/cxusb.c
@@ -16,6 +16,7 @@
* Copyright (C) 2005 Patrick Boettcher (patrick.boettc...@posteo.de)
* Copyright (C) 2006 Michael Krufky (mkru...@linuxtv.org)
* Copyright (C) 2006, 2007 Chris Pascoe (c.pas...@itee.uq.edu.au)
+ * Copyright (C) 2011, 2017 Maciej S. Szmigiero (m...@maciej.szmigiero.name)
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License as published by the Free
@@ -24,9 +25,12 @@
* see Documentation/dvb/README.dvb-usb for more information
*/
#include
-#include
-#include
+#include
+#include
#include
+#include
+#include
+#include
#include "cxusb.h"
@@ -47,17 +51,45 @@
#include "si2157.h"
/* debug */
-static int dvb_usb_cxusb_debug;
+int dvb_usb_cxusb_debug;
module_param_named(debug, dvb_usb_cxusb_debug, int, 0644);
-MODULE_PARM_DESC(debug, "set debugging level (1=rc (or-able))."
DVB_USB_DEBUG_STATUS);
+MODULE_PARM_DESC(debug, "set debugging level (see cxusb.h)."
+DVB_USB_DEBUG_STATUS);
DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
-#define deb_info(args...) dprintk(dvb_usb_cxusb_debug, 0x03, args)
-#define deb_i2c(args...)dprintk(dvb_usb_cxusb_debug, 0x02, args)
+#define deb_info(args...) dprintk(dvb_usb_cxusb_debug, CXUSB_DBG_MISC, args)
+#define deb_i2c(args...)dprintk(dvb_usb_cxusb_debug, CXUSB_DBG_I2C, args)
+
+enum cxusb_table_index {
+ MEDION_MD95700,
+ DVICO_BLUEBIRD_LG064F_COLD,
+ DVICO_BLUEBIRD_LG064F_WARM,
+ DVICO_BLUEBIRD_DUAL_1_COLD,
+ DVICO_BLUEBIRD_DUAL_1_WARM,
+ DVICO_BLUEBIRD_LGZ201_COLD,
+ DVICO_BLUEBIRD_LGZ201_WARM,
+ DVICO_BLUEBIRD_TH7579_COLD,
+ DVICO_BLUEBIRD_TH7579_WARM,
+ DIGITALNOW_BLUEBIRD_DUAL_1_COLD,
+ DIGITALNOW_BLUEBIRD_DUAL_1_WARM,
+ DVICO_BLUEBIRD_DUAL_2_COLD,
+ DVICO_BLUEBIRD_DUAL_2_WARM,
+ DVICO_BLUEBIRD_DUAL_4,
+ DVICO_BLUEBIRD_DVB_T_NANO_2,
+ DVICO_BLUEBIRD_DVB_T_NANO_2_NFW_WARM,
+ AVERMEDIA_VOLAR_A868R,
+ DVICO_BLUEBIRD_DUAL_4_REV_2,
+ CONEXANT_D680_DMB,
+ MYGICA_D689,
+ MYGICA_T230,
+ NR__cxusb_table_index
+};
+
+static struct usb_device_id cxusb_table[];
-static int cxusb_ctrl_msg(struct dvb_usb_device *d,
- u8 cmd, const u8 *wbuf, int wlen, u8 *rbuf, int rlen)
+int cxusb_ctrl_msg(struct dvb_usb_device *d,
+ u8 cmd, const u8 *wbuf, int wlen, u8 *rbuf, int rlen)
{
struct cxusb_state *st = d->priv;
int ret;
@@ -89,7 +121,8 @@ static void cxusb_gpio_tuner(struct dvb_usb_device *d, int
onoff)
struct cxusb_state *st = d->priv;
u8 o[2], i;
- if (st->gpio_write_state[GPIO_TUNER] == onoff)
+ if (st->gpio_write_state[GPIO_TUNER] == onoff &&
+ !st->gpio_write_refresh[GPIO_TUNER])
return;
o[0] = GPIO_TUNER;
@@ -100,6 +133,7 @@ static void cxusb_gpio_tuner(struct dvb_usb_device *d, int
onoff)
deb_info("gpio_write failed.\n");
st->gpio_write_state[GPIO_TUNER] = onoff;
+ st->gpio_write_refresh[GPIO_TUNER] = false;
}
static int cxusb_bluebird_gpio_rw(struct dvb_usb_device *d, u8 changemask,
@@ -259,7 +293,7 @@ static int cxusb_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg msg[],
static u32 cxusb_i2c_func(struct i2c_adapter *adapter)
{
- return I2C_FUNC_I2C;
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
}
static struct i2c_algorithm cxusb_i2c_algo = {
@@ -267,15 +301,48 @@ static struct i2c_algorithm cxusb_i2c_algo = {
.functionality = cxusb_i2c_func,
};
-static int cxusb_power_ctrl(struct dvb_usb_device *d, int onoff)
+static int _cxusb_power_ctrl(struct dvb_usb_device *d, int onoff)
{
u8 b = 0;
+
+ deb_info("setting power %s\n", onoff ? "ON" : "OFF");
+
if (onoff)
return cxusb_ctrl_msg(d, C
[PATCH v6 2/6] cx25840: add kernel-doc description of struct cx25840_state
This commit describes a device instance private data of the driver
(struct cx25840_state) in a kernel-doc style comment.
Signed-off-by: Maciej S. Szmigiero
---
drivers/media/i2c/cx25840/cx25840-core.h | 33 ++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/cx25840/cx25840-core.h
b/drivers/media/i2c/cx25840/cx25840-core.h
index fb13a624d2e3..c323b1af1f83 100644
--- a/drivers/media/i2c/cx25840/cx25840-core.h
+++ b/drivers/media/i2c/cx25840/cx25840-core.h
@@ -45,6 +45,35 @@ enum cx25840_media_pads {
CX25840_NUM_PADS
};
+/**
+ * struct cx25840_state - a device instance private data
+ * @c: i2c_client struct representing this device
+ * @sd:our V4L2 sub-device
+ * @hdl: our V4L2 control handler
+ * @volume:audio volume V4L2 control (non-cx2583x devices only)
+ * @mute: audio mute V4L2 control (non-cx2583x devices only)
+ * @pvr150_workaround: whether we enable workaround for Hauppauge PVR150
+ * hardware bug (audio dropping out)
+ * @radio: set if we are currently in the radio mode, otherwise
+ * the current mode is non-radio (that is, video)
+ * @std: currently set video standard
+ * @vid_input: currently set video input
+ * @aud_input: currently set audio input
+ * @audclk_freq: currently set audio sample rate
+ * @audmode: currently set audio mode (when in non-radio mode)
+ * @vbi_line_offset: vbi line number offset
+ * @id:exact device model
+ * @rev: raw device id read from the chip
+ * @is_initialized:whether we have already loaded firmware into the chip
+ * and initialized it
+ * @vbi_regs_offset: offset of vbi regs
+ * @fw_wait: wait queue to wake an initalization function up when
+ * firmware loading (on a separate workqueue) finishes
+ * @fw_work: a work that actually loads the firmware on a separate
+ * workqueue
+ * @ir_state: a pointer to chip IR controller private data
+ * @pads: array of supported chip pads (currently only a stub)
+ */
struct cx25840_state {
struct i2c_client *c;
struct v4l2_subdev sd;
@@ -66,8 +95,8 @@ struct cx25840_state {
u32 rev;
int is_initialized;
unsigned vbi_regs_offset;
- wait_queue_head_t fw_wait;/* wake up when the fw load is finished */
- struct work_struct fw_work; /* work entry for fw load */
+ wait_queue_head_t fw_wait;
+ struct work_struct fw_work;
struct cx25840_ir_state *ir_state;
#if defined(CONFIG_MEDIA_CONTROLLER)
struct media_padpads[CX25840_NUM_PADS];
[PATCH v6 1/6] ivtv: zero-initialize cx25840 platform data
We need to zero-initialize cx25840 platform data structure to make sure that its future members do not contain random stack garbage. Signed-off-by: Maciej S. Szmigiero --- drivers/media/pci/ivtv/ivtv-i2c.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/pci/ivtv/ivtv-i2c.c b/drivers/media/pci/ivtv/ivtv-i2c.c index 522cd111e399..e9ce54dd5e01 100644 --- a/drivers/media/pci/ivtv/ivtv-i2c.c +++ b/drivers/media/pci/ivtv/ivtv-i2c.c @@ -293,6 +293,7 @@ int ivtv_i2c_register(struct ivtv *itv, unsigned idx) .platform_data = , }; + memset(, 0, sizeof(pdata)); pdata.pvr150_workaround = itv->pvr150_workaround; sd = v4l2_i2c_new_subdev_board(>v4l2_dev, adap, _info, NULL);
[PATCH v6 4/6] tuner-simple: allow setting mono radio mode
For some types of tuners (Philips FMD1216ME(X) MK3 currently) we know that letting TDA9887 output port 1 remain high (inactive) will switch FM radio to mono mode. Let's make use of this functionality - nothing changes for the default stereo radio mode. Tested on a Medion 95700 board which has a FMD1216ME tuner. Signed-off-by: Maciej S. Szmigiero --- drivers/media/tuners/tuner-simple.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/tuners/tuner-simple.c b/drivers/media/tuners/tuner-simple.c index 36b88f820239..29c1473f2e9f 100644 --- a/drivers/media/tuners/tuner-simple.c +++ b/drivers/media/tuners/tuner-simple.c @@ -670,6 +670,7 @@ static int simple_set_radio_freq(struct dvb_frontend *fe, int rc, j; struct tuner_params *t_params; unsigned int freq = params->frequency; + bool mono = params->audmode == V4L2_TUNER_MODE_MONO; tun = priv->tun; @@ -736,8 +737,8 @@ static int simple_set_radio_freq(struct dvb_frontend *fe, config |= TDA9887_PORT2_ACTIVE; if (t_params->intercarrier_mode) config |= TDA9887_INTERCARRIER; -/* if (t_params->port1_set_for_fm_mono) - config &= ~TDA9887_PORT1_ACTIVE;*/ + if (t_params->port1_set_for_fm_mono && mono) + config &= ~TDA9887_PORT1_ACTIVE; if (t_params->fm_gain_normal) config |= TDA9887_GAIN_NORMAL; if (t_params->radio_if == 2)
[PATCH v6 6/6] [media] cxusb: add analog mode support for Medion MD95700
This patch adds support for analog part of Medion 95700 in the cxusb
driver.
What works:
* Video capture at various sizes with sequential fields,
* Input switching (TV Tuner, Composite, S-Video),
* TV and radio tuning,
* Video standard switching and auto detection,
* Radio mode switching (stereo / mono),
* Unplugging while capturing,
* DVB / analog coexistence,
* Raw BT.656 stream support.
What does not work yet:
* Audio,
* VBI,
* Picture controls.
Signed-off-by: Maciej S. Szmigiero
---
drivers/media/usb/dvb-usb/Kconfig| 16 +-
drivers/media/usb/dvb-usb/Makefile |3 +
drivers/media/usb/dvb-usb/cxusb-analog.c | 1914 ++
drivers/media/usb/dvb-usb/cxusb.c|2 -
drivers/media/usb/dvb-usb/cxusb.h| 106 ++
5 files changed, 2037 insertions(+), 4 deletions(-)
create mode 100644 drivers/media/usb/dvb-usb/cxusb-analog.c
diff --git a/drivers/media/usb/dvb-usb/Kconfig
b/drivers/media/usb/dvb-usb/Kconfig
index 2651ae277347..b4ef8f6eb470 100644
--- a/drivers/media/usb/dvb-usb/Kconfig
+++ b/drivers/media/usb/dvb-usb/Kconfig
@@ -138,12 +138,24 @@ config DVB_USB_CXUSB
select MEDIA_TUNER_SI2157 if MEDIA_SUBDRV_AUTOSELECT
help
Say Y here to support the Conexant USB2.0 hybrid reference design.
- Currently, only DVB and ATSC modes are supported, analog mode
- shall be added in the future. Devices that require this module:
+ DVB and ATSC modes are supported, for a basic analog mode support
+ see the next option ("Analog support for the Conexant USB2.0 hybrid
+ reference design").
+ Devices that require this module:
Medion MD95700 hybrid USB2.0 device.
DViCO FusionHDTV (Bluebird) USB2.0 devices
+config DVB_USB_CXUSB_ANALOG
+ bool "Analog support for the Conexant USB2.0 hybrid reference design"
+ depends on DVB_USB_CXUSB && VIDEO_V4L2
+ select VIDEO_CX25840
+ select VIDEOBUF2_VMALLOC
+ help
+ Say Y here to enable basic analog mode support for the Conexant
+ USB2.0 hybrid reference design.
+ Currently this mode is supported only on a Medion MD95700 device.
+
config DVB_USB_M920X
tristate "Uli m920x DVB-T USB2.0 support"
depends on DVB_USB
diff --git a/drivers/media/usb/dvb-usb/Makefile
b/drivers/media/usb/dvb-usb/Makefile
index 9ad2618408ef..e47bcadcfc3d 100644
--- a/drivers/media/usb/dvb-usb/Makefile
+++ b/drivers/media/usb/dvb-usb/Makefile
@@ -42,6 +42,9 @@ dvb-usb-digitv-objs := digitv.o
obj-$(CONFIG_DVB_USB_DIGITV) += dvb-usb-digitv.o
dvb-usb-cxusb-objs := cxusb.o
+ifeq ($(CONFIG_DVB_USB_CXUSB_ANALOG),y)
+dvb-usb-cxusb-objs += cxusb-analog.o
+endif
obj-$(CONFIG_DVB_USB_CXUSB) += dvb-usb-cxusb.o
dvb-usb-ttusb2-objs := ttusb2.o
diff --git a/drivers/media/usb/dvb-usb/cxusb-analog.c
b/drivers/media/usb/dvb-usb/cxusb-analog.c
new file mode 100644
index ..969d82b24f41
--- /dev/null
+++ b/drivers/media/usb/dvb-usb/cxusb-analog.c
@@ -0,0 +1,1914 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// DVB USB compliant linux driver for Conexant USB reference design -
+// (analog part).
+//
+// Copyright (C) 2011, 2017 Maciej S. Szmigiero (m...@maciej.szmigiero.name)
+//
+// TODO:
+// * audio support,
+// * finish radio support (requires audio of course),
+// * VBI support,
+// * controls support
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "cxusb.h"
+
+static int cxusb_medion_v_queue_setup(struct vb2_queue *q,
+ unsigned int *num_buffers,
+ unsigned int *num_planes,
+ unsigned int sizes[],
+ struct device *alloc_devs[])
+{
+ struct dvb_usb_device *dvbdev = vb2_get_drv_priv(q);
+ struct cxusb_medion_dev *cxdev = dvbdev->priv;
+ unsigned int size = cxdev->raw_mode ?
+ CXUSB_VIDEO_MAX_FRAME_SIZE :
+ cxdev->width * cxdev->height * 2;
+
+ if (*num_planes > 0) {
+ if (*num_planes != 1)
+ return -EINVAL;
+
+ if (sizes[0] < size)
+ return -EINVAL;
+ } else {
+ *num_planes = 1;
+ sizes[0] = size;
+ }
+
+ if (q->num_buffers + *num_buffers < 6)
+ *num_buffers = 6 - q->num_buffers;
+
+ return 0;
+}
+
+static int cxusb_medion_v_buf_init(struct vb2_buffer *vb)
+{
+ struct dvb_usb_device *dvbdev = vb2_get_drv_priv(vb->vb2_queue);
+ struct cxusb_medion_dev *cxdev = dvbdev->priv;
+
+ cxusb_vprintk(dvbdev, OPS, "buffer init\n");
+
+ if (cxdev->raw_mode) {
+ if (vb2_plane_size(vb, 0) < CXUSB_VIDEO_MAX_FRAME_SIZE)
+
[PATCH v6 3/6] cx25840: add pin to pad mapping and output format configuration
This commit adds pin to pad mapping and output format configuration support
in CX2584x-series chips to cx25840 driver.
This functionality is then used to allow disabling ivtv-specific hacks
(called a "generic mode"), so cx25840 driver can be used for other devices
not needing them without risking compatibility problems.
Signed-off-by: Maciej S. Szmigiero
---
drivers/media/i2c/cx25840/cx25840-core.c | 396 ++-
drivers/media/i2c/cx25840/cx25840-core.h | 13 +
drivers/media/i2c/cx25840/cx25840-vbi.c | 3 +
include/media/drv-intf/cx25840.h | 74 +-
4 files changed, 484 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/cx25840/cx25840-core.c
b/drivers/media/i2c/cx25840/cx25840-core.c
index b168bf3635b6..7dc3d0870808 100644
--- a/drivers/media/i2c/cx25840/cx25840-core.c
+++ b/drivers/media/i2c/cx25840/cx25840-core.c
@@ -21,6 +21,9 @@
* CX23888 DIF support for the HVR1850
* Copyright (C) 2011 Steven Toth
*
+ * CX2584x pin to pad mapping and output format configuration support are
+ * Copyright (C) 2011 Maciej S. Szmigiero
+ *
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
@@ -316,6 +319,260 @@ static int cx23885_s_io_pin_config(struct v4l2_subdev
*sd, size_t n,
return 0;
}
+static u8 cx25840_function_to_pad(struct i2c_client *client, u8 function)
+{
+ switch (function) {
+ case CX25840_PAD_ACTIVE:
+ return 1;
+
+ case CX25840_PAD_VACTIVE:
+ return 2;
+
+ case CX25840_PAD_CBFLAG:
+ return 3;
+
+ case CX25840_PAD_VID_DATA_EXT0:
+ return 4;
+
+ case CX25840_PAD_VID_DATA_EXT1:
+ return 5;
+
+ case CX25840_PAD_GPO0:
+ return 6;
+
+ case CX25840_PAD_GPO1:
+ return 7;
+
+ case CX25840_PAD_GPO2:
+ return 8;
+
+ case CX25840_PAD_GPO3:
+ return 9;
+
+ case CX25840_PAD_IRQ_N:
+ return 10;
+
+ case CX25840_PAD_AC_SYNC:
+ return 11;
+
+ case CX25840_PAD_AC_SDOUT:
+ return 12;
+
+ case CX25840_PAD_PLL_CLK:
+ return 13;
+
+ case CX25840_PAD_VRESET:
+ return 14;
+
+ default:
+ if (function != CX25840_PAD_DEFAULT)
+ v4l_err(client,
+ "invalid function %u, assuming default\n",
+ (unsigned int)function);
+ return 0;
+ }
+}
+
+static void cx25840_set_invert(u8 *pinctrl3, u8 *voutctrl4, u8 function,
+ u8 pin, bool invert)
+{
+ switch (function) {
+ case CX25840_PAD_IRQ_N:
+ if (invert)
+ *pinctrl3 &= ~2;
+ else
+ *pinctrl3 |= 2;
+ break;
+
+ case CX25840_PAD_ACTIVE:
+ if (invert)
+ *voutctrl4 |= BIT(2);
+ else
+ *voutctrl4 &= ~BIT(2);
+ break;
+
+ case CX25840_PAD_VACTIVE:
+ if (invert)
+ *voutctrl4 |= BIT(5);
+ else
+ *voutctrl4 &= ~BIT(5);
+ break;
+
+ case CX25840_PAD_CBFLAG:
+ if (invert)
+ *voutctrl4 |= BIT(4);
+ else
+ *voutctrl4 &= ~BIT(4);
+ break;
+
+ case CX25840_PAD_VRESET:
+ if (invert)
+ *voutctrl4 |= BIT(0);
+ else
+ *voutctrl4 &= ~BIT(0);
+ break;
+ }
+
+ if (function != CX25840_PAD_DEFAULT)
+ return;
+
+ switch (pin) {
+ case CX25840_PIN_DVALID_PRGM0:
+ if (invert)
+ *voutctrl4 |= BIT(6);
+ else
+ *voutctrl4 &= ~BIT(6);
+ break;
+
+ case CX25840_PIN_HRESET_PRGM2:
+ if (invert)
+ *voutctrl4 |= BIT(1);
+ else
+ *voutctrl4 &= ~BIT(1);
+ break;
+ }
+}
+
+static int cx25840_s_io_pin_config(struct v4l2_subdev *sd, size_t n,
+ struct v4l2_subdev_io_pin_config *p)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ unsigned int i;
+ u8 pinctrl[6], pinconf[10], voutctrl4;
+
+ for (i = 0; i < 6; i++)
+ pinctrl[i] = cx25840_read(client, 0x114 + i);
+
+ for (i = 0; i < 10; i++)
+ pinconf[i] = cx25840_read(client, 0x11c + i);
+
+ voutctrl4 = cx25840_read(client, 0x407);
+
+ for (i = 0; i < n; i++) {
+ u8
[PATCH] x86/microcode/AMD: check microcode file sanity before loading it
Currently, it is very easy to make the AMD microcode update driver crash
or spin on a malformed microcode file since it does very little
consistency checking on data loaded from such file.
This commit introduces various checks, mostly on length-type fields, so
all corrupted microcode files are (hopefully) correctly rejected instead.
Signed-off-by: Maciej S. Szmigiero
---
Test files are at https://pastebin.com/XkKUSmMp
One has to enable KASAN in the kernel config and rename a particular
test file to name appropriate to the running CPU family to test its
loading.
arch/x86/include/asm/microcode_amd.h | 6 ++
arch/x86/kernel/cpu/microcode/amd.c | 111 ++-
2 files changed, 89 insertions(+), 28 deletions(-)
diff --git a/arch/x86/include/asm/microcode_amd.h
b/arch/x86/include/asm/microcode_amd.h
index 209492849566..e797d5d939d7 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -19,6 +19,9 @@ struct equiv_cpu_entry {
u16 res;
} __attribute__((packed));
+/* 4k */
+#define UCODE_EQUIV_CPU_TABLE_MAX_SIZE (256 * sizeof(struct equiv_cpu_entry))
+
struct microcode_header_amd {
u32 data_code;
u32 patch_id;
@@ -41,6 +44,9 @@ struct microcode_amd {
unsigned intmpb[0];
};
+/* if a patch is larger than this the microcode file is surely corrupted */
+#define PATCH_MAX_SIZE_ABSOLUTE (16 * 1024 * 1024)
+
#define PATCH_MAX_SIZE PAGE_SIZE
#ifdef CONFIG_MICROCODE_AMD
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index a998e1a7d46f..f798bd4004aa 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -39,6 +39,7 @@
#include
static struct equiv_cpu_entry *equiv_cpu_table;
+static unsigned int equiv_cpu_table_size;
/*
* This points to the current valid container of microcode patches which we
will
@@ -63,12 +64,17 @@ static u8 amd_ucode_patch[PATCH_MAX_SIZE];
static const char
ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";
-static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
+static u16 find_equiv_id(const struct equiv_cpu_entry *equiv_table,
+unsigned int equiv_table_size, u32 sig)
{
- for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
- if (sig == equiv_table->installed_cpu)
- return equiv_table->equiv_cpu;
- }
+ unsigned int i;
+
+ if (!equiv_table)
+ return 0;
+
+ for (i = 0; i < equiv_table_size && equiv_table[i].installed_cpu; i++)
+ if (sig == equiv_table[i].installed_cpu)
+ return equiv_table[i].equiv_cpu;
return 0;
}
@@ -80,29 +86,41 @@ static u16 find_equiv_id(struct equiv_cpu_entry
*equiv_table, u32 sig)
* Returns the amount of bytes consumed while scanning. @desc contains all the
* data we're going to use in later stages of the application.
*/
-static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
+static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
{
struct equiv_cpu_entry *eq;
- ssize_t orig_size = size;
+ size_t orig_size = size;
u32 *hdr = (u32 *)ucode;
+ u32 eqsize;
u16 eq_id;
u8 *buf;
+ if (size < CONTAINER_HDR_SZ)
+ return 0;
+
/* Am I looking at an equivalence table header? */
if (hdr[0] != UCODE_MAGIC ||
hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
hdr[2] == 0)
return CONTAINER_HDR_SZ;
+ eqsize = hdr[2];
+ if (eqsize < sizeof(*eq) ||
+ eqsize > UCODE_EQUIV_CPU_TABLE_MAX_SIZE)
+ return 0;
+
+ if (size < CONTAINER_HDR_SZ + eqsize)
+ return 0;
+
buf = ucode;
eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
/* Find the equivalence ID of our CPU in this table: */
- eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
+ eq_id = find_equiv_id(eq, eqsize / sizeof(*eq), desc->cpuid_1_eax);
- buf += hdr[2] + CONTAINER_HDR_SZ;
- size -= hdr[2] + CONTAINER_HDR_SZ;
+ buf += eqsize + CONTAINER_HDR_SZ;
+ size -= eqsize + CONTAINER_HDR_SZ;
/*
* Scan through the rest of the container to find where it ends. We do
@@ -112,6 +130,9 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
struct microcode_amd *mc;
u32 patch_size;
+ if (size < SECTION_HDR_SIZE)
+ break;
+
hdr = (u32 *)buf;
if (hdr[0] != UCODE_UCODE_TYPE)
@@ -126,6 +147,10 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
buf +=
Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it
On 10.03.2018 10:18, Borislav Petkov wrote: > On Sat, Mar 10, 2018 at 01:34:45AM +0100, Maciej S. Szmigiero wrote: >> Currently, it is very easy to make the AMD microcode update driver crash >> or spin on a malformed microcode file since it does very little >> consistency checking on data loaded from such file. > > Sorry but if one has enough permissions to install malformed microcode, > crashing the loader is the least of your problems. IOW, I don't see the > justification for the unnecessary complication with all those checks. While I agree this is not a security problem, I cannot agree that these checks are unnecessary driver complication. First, these checks are really just very basic checks like "check whether the loaded file is long enough to actually contain some structure before accessing it" or "don't iterate an array in file without checking if it actually has a terminating element" or "check whether microcode patch length isn't something like 2GB before allocating memory for it". Without them, it is easy to crash the driver when just playing with microcode files (and it turns out that AMD-released microcode files in linux-firmware actually don't contain the newest microcode versions, even for older CPUs). Second, since these checks happen only on a microcode file load (something that 99.9% of systems probably will do just once at boot time) it is hardly a performance-critical path. Third, we still do check consistency of data provided to various root-only syscalls (and these might be much more performance-critical than this code). > Thx. > Thanks, Maciej
Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it
On 10.03.2018 14:12, Borislav Petkov wrote: > On Sat, Mar 10, 2018 at 01:26:00PM +0100, Maciej S. Szmigiero wrote: >> Without them, it is easy to crash the driver when just playing with >> microcode files > > You're not supposed to play with the microcode files. If you do and > something breaks, you get to keep the pieces. > > If the official microcode files have a problem, then I'm all ears. > Anything else contrived which doesn't actually happen unless someone > manipulates them is not an issue the microcode loader should protect > against. > So, shall we remove data consistency checks of various root-only syscalls then? :) Maciej
Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it
On 10.03.2018 01:34, Maciej S. Szmigiero wrote: > Currently, it is very easy to make the AMD microcode update driver crash > or spin on a malformed microcode file since it does very little > consistency checking on data loaded from such file. > > This commit introduces various checks, mostly on length-type fields, so > all corrupted microcode files are (hopefully) correctly rejected instead. > > Signed-off-by: Maciej S. Szmigiero To make sure that it is clear what this patch is about: It *isn't* about verifying the actual microcode update, that is, the blob that gets sent to the CPU as the new microcode. Such verification is (hopefully) done by the CPU itself. It is about verifying *this driver-specific* container file that includes many microcode updates for different CPUs of a particular CPU family, along with metadata that helps the driver select the right microcode update to actually sent to the CPU. The microcode container files in linux-firmware don't contain the newest microcode versions, even for older CPUs. They are generally updated VERY rarely - I can see only three updates for them: on 2016-03-18, 2014-11-30 and 2013-07-11. There is no container file at all for family 17h (Zen) so distributions like OpenSUSE that include this file must have gotten it from some other source (or made from raw updates themselves). That's why to get things like IBPB it is sometimes necessary to use a newer microcode version than included in linux-firmware, sourced for example from a BIOS update. Since BIOS updates contain only actual (raw) microcode updates one has to place it in a microcode container file so this driver can parse it. As far I know there is no tool to automate this work so one has to manually tweak the container metadata. And this is prone to mistakes this patch protects against. Sorry for not making this 100% clear in the commit log. Maciej
Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it
On 10.03.2018 17:46, Borislav Petkov wrote: > On Sat, Mar 10, 2018 at 05:16:40PM +0100, Maciej S. Szmigiero wrote: (..) >> There is no container file at all for family 17h (Zen) so >> distributions like OpenSUSE that include this file must have gotten it >> from some other source > > Or maybe they've gotten it from AMD directly. Don't you think that > getting microcode from the CPU vendor directly is the logical thing? "some other source" than linux-firmware includes the CPU vendor. Also please note that while OpenSUSE can get the microcode directly from the CPU vendor there seems to be no official AMD web site that distributes microcode. And it looks like other distros simply get it from OpenSUSE: https://bugs.archlinux.org/task/56951 >> That's why to get things like IBPB it is sometimes necessary to use >> a newer microcode version than included in linux-firmware, sourced for >> example from a BIOS update. > > linux-firmware will get F17h microcode soon. Great! Hope it will include latest production versions for the whole family 17h. >> Since BIOS updates contain only actual (raw) microcode updates one >> has to place it in a microcode container file so this driver can parse >> it. >> >> As far I know there is no tool to automate this work so one has to >> manually tweak the container metadata. > > Let me get this straight: am I reading this correctly that you've tried > to carve out the F17h microcode from a BIOS update blob and you're > trying to load that?!? > > If so, you could've simply taken a distro microcode package and used > F17h microcode from there - they are all the same. > "microcode_amd_fam17h.bin" from both my distro (Gentoo) and OpenSUSE only contains family 23 model 2 microcode while my Ryzen is model 1. And my motherboard BIOS-loaded microcode is too old to contain IBPB support. Maciej
Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it
On 11.03.2018 10:59, Ingo Molnar wrote:
>
> * Maciej S. Szmigiero wrote:
>
>> Currently, it is very easy to make the AMD microcode update driver crash
>> or spin on a malformed microcode file since it does very little
>> consistency checking on data loaded from such file.
>>
>> This commit introduces various checks, mostly on length-type fields, so
>> all corrupted microcode files are (hopefully) correctly rejected instead.
>>
>> Signed-off-by: Maciej S. Szmigiero
>> ---
>> Test files are at https://pastebin.com/XkKUSmMp
>> One has to enable KASAN in the kernel config and rename a particular
>> test file to name appropriate to the running CPU family to test its
>> loading.
>>
>> arch/x86/include/asm/microcode_amd.h | 6 ++
>> arch/x86/kernel/cpu/microcode/amd.c | 111
>> ++-
>> 2 files changed, 89 insertions(+), 28 deletions(-)
>
> Treating microcode update files as external input and sanity checking the
> data
> makes sense I suppose, but there's various small uglies in the patch:
>
>> +/* if a patch is larger than this the microcode file is surely corrupted */
>> +#define PATCH_MAX_SIZE_ABSOLUTE (16 * 1024 * 1024)
>
> Please capitalize comments.
Will do.
>
>> * Returns the amount of bytes consumed while scanning. @desc contains all
>> the
>> * data we're going to use in later stages of the application.
>> */
>> -static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc
>> *desc)
>> +static size_t parse_container(u8 *ucode, size_t size, struct cont_desc
>> *desc)
>> {
>> struct equiv_cpu_entry *eq;
>> -ssize_t orig_size = size;
>> +size_t orig_size = size;
>> u32 *hdr = (u32 *)ucode;
>> +u32 eqsize;
>> u16 eq_id;
>> u8 *buf;
>
> So we have 'eq_id', but 'eqsize'? Why not 'eq_size' to have fewer random
> variations of coding style?
Will change.
>>
>> +if (size < CONTAINER_HDR_SZ)
>> +return 0;
>
> The comment about CONTAINER_HDR_SZ better belongs here, where we use it.
Will move it.
>> /* Am I looking at an equivalence table header? */
>> if (hdr[0] != UCODE_MAGIC ||
>> hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
>> hdr[2] == 0)
>> return CONTAINER_HDR_SZ;
>>
>> +eqsize = hdr[2];
>> +if (eqsize < sizeof(*eq) ||
>> +eqsize > UCODE_EQUIV_CPU_TABLE_MAX_SIZE)
>> +return 0;
>> +
>> +if (size < CONTAINER_HDR_SZ + eqsize)
>> +return 0;
>> +
>> buf = ucode;
>>
>> eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
>>
>> /* Find the equivalence ID of our CPU in this table: */
>> -eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
>> +eq_id = find_equiv_id(eq, eqsize / sizeof(*eq), desc->cpuid_1_eax);
>
> Does eq_size have to be a multiple of sizeof(*eq)? If yes then we should
> probably
> check that too.
In principle yes, but having garbage at the end of the equivalence
table in a microcode container file is a non-fatal problem.
I have mixed feelings whether we should be really strict here, especially
that the existing driver would accept such files without any error.
>> -static int install_equiv_cpu_table(const u8 *buf)
>> +static int install_equiv_cpu_table(const u8 *buf, size_t bufsize)
>
> s/bufsize/buf_size
Will change.
>> {
>> unsigned int *ibuf = (unsigned int *)buf;
>> -unsigned int type = ibuf[1];
>> -unsigned int size = ibuf[2];
>> +unsigned int type, size;
>>
>> -if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
>> -pr_err("empty section/"
>> - "invalid type field in container file section header\n");
>> +if (bufsize < CONTAINER_HDR_SZ) {
>> +pr_err("no container header\n");
>> +return -EINVAL;
>> +}
>> +
>> +type = ibuf[1];
>> +if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
>> +pr_err("invalid type field in container file section header\n");
>> +return -EINVAL;
>> +}
>> +
>> +size = ibuf[2];
>> +if (size < sizeof(struct equiv_cpu_entry) ||
>> +size > UCODE_EQUIV_CPU_TABLE_MAX_SIZE) {
>> +pr_err("equivalent CPU table size invalid\n");
>> +return -EINVAL;
>> +}
>> +
>> +if (bufsize < CONTAINER_HDR_SZ + size) {
>> +pr_err("equivalent CPU table truncated\n");
>> return -EINVAL;
>> }
>>
>> @@ -559,6 +599,7 @@ static int install_equiv_cpu_table(const u8 *buf)
>> }
>>
>> memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
>> +equiv_cpu_table_size = size / sizeof(struct equiv_cpu_entry);
>
> Btw., 'equiv_cpu_table_size' is an ambiguous name: often _size variables are
> in
> byte units - but this is number of entries. So the name should be
> 'equiv_cpu_table_entries' or so.
Will rename.
> Thanks,
>
> Ingo
>
Thanks,
Maciej
[PATCH v2] x86/microcode/AMD: check microcode file sanity before loading it
Currently, it is very easy to make the AMD microcode update driver crash
or spin on a malformed microcode file since it does very little
consistency checking on data loaded from such file.
This commit introduces various checks, mostly on length-type fields, so
all corrupted microcode files are (hopefully) correctly rejected instead.
Signed-off-by: Maciej S. Szmigiero
---
Changes from v1: Capitalize a comment, rename 'eqsize' and 'bufsize'
to 'eq_size' and 'buf_size', respectively, attach a comment about
checking the equivalence table header to its first size check, rename
'equiv{_cpu,}_table_size' to 'equiv{_cpu,}_table_entries'.
Test files are at https://pastebin.com/XkKUSmMp
One has to enable KASAN in the kernel config and rename a particular
test file to name appropriate to the running CPU family to test its
loading.
arch/x86/include/asm/microcode_amd.h | 6 ++
arch/x86/kernel/cpu/microcode/amd.c | 112 ++-
2 files changed, 90 insertions(+), 28 deletions(-)
diff --git a/arch/x86/include/asm/microcode_amd.h
b/arch/x86/include/asm/microcode_amd.h
index 209492849566..373a202ea569 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -19,6 +19,9 @@ struct equiv_cpu_entry {
u16 res;
} __attribute__((packed));
+/* 4k */
+#define UCODE_EQUIV_CPU_TABLE_MAX_SIZE (256 * sizeof(struct equiv_cpu_entry))
+
struct microcode_header_amd {
u32 data_code;
u32 patch_id;
@@ -41,6 +44,9 @@ struct microcode_amd {
unsigned intmpb[0];
};
+/* If a patch is larger than this the microcode file is surely corrupted */
+#define PATCH_MAX_SIZE_ABSOLUTE (16 * 1024 * 1024)
+
#define PATCH_MAX_SIZE PAGE_SIZE
#ifdef CONFIG_MICROCODE_AMD
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index a998e1a7d46f..1cbccf79ff68 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -39,6 +39,7 @@
#include
static struct equiv_cpu_entry *equiv_cpu_table;
+static unsigned int equiv_cpu_table_entries;
/*
* This points to the current valid container of microcode patches which we
will
@@ -63,12 +64,18 @@ static u8 amd_ucode_patch[PATCH_MAX_SIZE];
static const char
ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";
-static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
+static u16 find_equiv_id(const struct equiv_cpu_entry *equiv_table,
+unsigned int equiv_table_entries, u32 sig)
{
- for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
- if (sig == equiv_table->installed_cpu)
- return equiv_table->equiv_cpu;
- }
+ unsigned int i;
+
+ if (!equiv_table)
+ return 0;
+
+ for (i = 0; i < equiv_table_entries && equiv_table[i].installed_cpu;
+i++)
+ if (sig == equiv_table[i].installed_cpu)
+ return equiv_table[i].equiv_cpu;
return 0;
}
@@ -80,29 +87,41 @@ static u16 find_equiv_id(struct equiv_cpu_entry
*equiv_table, u32 sig)
* Returns the amount of bytes consumed while scanning. @desc contains all the
* data we're going to use in later stages of the application.
*/
-static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
+static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
{
struct equiv_cpu_entry *eq;
- ssize_t orig_size = size;
+ size_t orig_size = size;
u32 *hdr = (u32 *)ucode;
+ u32 eq_size;
u16 eq_id;
u8 *buf;
/* Am I looking at an equivalence table header? */
+ if (size < CONTAINER_HDR_SZ)
+ return 0;
+
if (hdr[0] != UCODE_MAGIC ||
hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
hdr[2] == 0)
return CONTAINER_HDR_SZ;
+ eq_size = hdr[2];
+ if (eq_size < sizeof(*eq) ||
+ eq_size > UCODE_EQUIV_CPU_TABLE_MAX_SIZE)
+ return 0;
+
+ if (size < CONTAINER_HDR_SZ + eq_size)
+ return 0;
+
buf = ucode;
eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
/* Find the equivalence ID of our CPU in this table: */
- eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
+ eq_id = find_equiv_id(eq, eq_size / sizeof(*eq), desc->cpuid_1_eax);
- buf += hdr[2] + CONTAINER_HDR_SZ;
- size -= hdr[2] + CONTAINER_HDR_SZ;
+ buf += eq_size + CONTAINER_HDR_SZ;
+ size -= eq_size + CONTAINER_HDR_SZ;
/*
* Scan through the rest of the container to find where it ends. We do
@@ -112,6 +131,9 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
struct microcode_amd *mc;
u32 p
Re: [PATCH v2] x86/microcode/AMD: check microcode file sanity before loading it
On 12.03.2018 10:53, Borislav Petkov wrote: > On Sun, Mar 11, 2018 at 04:27:03PM +0100, Maciej S. Szmigiero wrote: >> +/* 4k */ >> +#define UCODE_EQUIV_CPU_TABLE_MAX_SIZE (256 * sizeof(struct >> equiv_cpu_entry)) > > And you came up with that max size how exactly? The equivalent CPU table is allocated using vmalloc() so it is nice when the maximum size is an integer multiple of the page size. Since the maximum entry count in current microcode files is 18 the maximum size of 256 entries (or one page) gives us plenty of headroom. Also, looking in the past, there probably won't be more than 256 AMD CPU types in one CPU family. > >> +/* If a patch is larger than this the microcode file is surely corrupted */ >> +#define PATCH_MAX_SIZE_ABSOLUTE (16 * 1024 * 1024) > > Same question here. > This limit is an absolute upper cap of a patch size. This number is high so it won't have to be changed in the future, it only serves as a plausibility check before we consider other data contained in the particular patch. Current patches are 4k max size, but since the size field is 32 bit-wide one can theoretically specify sizes up to 4GB. Since these two maximum sizes are somewhat arbitrary if anybody wants to propose other values the patch can be updated, naturally. Maciej
Re: [PATCH v2] x86/microcode/AMD: check microcode file sanity before loading it
On 12.03.2018 14:06, Borislav Petkov wrote: > On Mon, Mar 12, 2018 at 01:56:59PM +0100, Maciej S. Szmigiero wrote: (..) >> Since the maximum entry count in current microcode files is 18 the > > Where did you dream up that 18? "microcode_amd.bin" in linux-firmware. >> Also, looking in the past, there probably won't be more than 256 AMD CPU >> types in one CPU family. > > Wrong. There is no problem raising this value in that (future) case. As I wrote previously, currently the maximum used count is 18. > The only limitation on the equivalence table size we have is the 32-bit > unsigned length field at offset 8 in the equivalence table header. Not really, since even in the existing code CONTAINER_HDR_SZ (12) gets added to this size, then the sum is cast to a (signed) int. If this value is negative then the file get rejected. >> This limit is an absolute upper cap of a patch size. > > More dreamt up crap. > > See verify_patch_size() for the actual patch sizes. > It can be changed to the current maximum across sizes for particular families, but then the limit will need to be raised when adding a new family (if it uses a larger patch). Maciej
Re: [PATCH v2] x86/microcode/AMD: check microcode file sanity before loading it
On 12.03.2018 14:48, Borislav Petkov wrote: > On Mon, Mar 12, 2018 at 02:32:30PM +0100, Maciej S. Szmigiero wrote: >> "microcode_amd.bin" in linux-firmware. > > That is the microcode container for all families < 0x15. And it > *happens* to have 18 entries. > > So purely arbitrary: > > Equivalence table (magic: AMD, type: 0, length: 288 (0x120)) (the long table was cut) > >> There is no problem raising this value in that (future) case. >> As I wrote previously, currently the maximum used count is 18. > > There is a problem because not everyone can upgrade their kernels > like you. Distros and big deployments can't just up and update their > kernels at a whim just because you imposed an arbitrary limit which you > determined would be ok. First, this limit is more than 14 times higher than the current maximum count. And this current maximum was reached by CPU types added in families < 15h during last 10+ years (the oldest supported CPU family in this container is 10h, which - according to Wikipedia - was released September 10, 2007). At that rate exceeding this limit will take 130+ additional years (and this assumes that AMD will introduce new CPU types in these families at the same rate as in the last 10 years - I sincerely doubt it). If somebody does not update their kernel for 130 years (even to -stable versions) then he or she has a much bigger problem than incompatibility with the current microcode update file. > >> Not really, since even in the existing code CONTAINER_HDR_SZ (12) gets >> added to this size, then the sum is cast to a (signed) int. >> If this value is negative then the file get rejected. > > That is a bug in install_equiv_cpu_table() - it should return unsigned int. > >> It can be changed to the current maximum across sizes for particular > > What is the "current maximum across sizes"? > This is the current maximum patch size across families: #define F15H_MPB_MAX_SIZE 4096 Maciej
Re: [PATCH v4 06/10] x86/microcode/AMD: Check patch size in verify_and_add_patch()
On 22.03.2018 17:11, Borislav Petkov wrote:
> On Fri, Mar 16, 2018 at 12:08:17AM +0100, Maciej S. Szmigiero wrote:
>> @@ -505,7 +505,7 @@ static unsigned int verify_patch_size(u8 family, u32
>> patch_size,
>> break;
>> }
>>
>> -if (patch_size > min_t(u32, size, max_size)) {
>> +if (patch_size > min_t(size_t, size, max_size)) {
>
> So I don't like this conversion to 8-byte-width size_t's. It is not
> necessary. I'm pretty sure we can do fine with signed and unsigned ints.
It is possible to keep verify_patch_size() unmodified (with unsigned int
and u32) but microcode container files >4GB in size then may be rejected,
even if they are technically valid (while a bit unrealistic) on 64-bit
kernels.
Thanks,
Maciej
Re: [PATCH] X.509: unpack RSA signatureValue field from BIT STRING
On 07.03.2018 18:56, Maciej S. Szmigiero wrote:
> On 07.03.2018 16:44, David Howells wrote:
>> Maciej S. Szmigiero wrote:
>>
>>> + if (!strcmp(ctx->cert->sig->pkey_algo, "rsa")) {
>>
>> I'm going to change this to '== 0' rather than '!'.
>
> No problem.
I cannot find this patch in any tree that I have looked at.
Are you going to pick it up later or am I not looking at the right
place?
Thanks,
Maciej
Re: [PATCH] x86/speculation: Fill the RSB on context switch also on non-IBPB CPUs
On 22.03.2018 16:46, Dave Hansen wrote: > On 03/21/2018 05:09 PM, Maciej S. Szmigiero wrote: >> As far as I understand the issue this should provide a good protection >> for userspace processes that were recompiled with retpolines as they >> won't have any indirect jumps and calls. > > Instead of saying "good protection", let's just say that it could > mitigate attacks that require consumption of attacker-placed RSB entries. All right. >>> Do you perhaps want to do RSB manipulation in lieu of IBPB when >>> switching *to* a non-dumpable process and IBPB is not available? >> >> Is it worth differentiating such processes in this case? >> IBPB is supposed to be very expensive so certainly it is worthwhile >> to do it only for high-value processes (=non-dumpable). >> >> However, it is unlikely that existing RSB entries from the previous >> task match the new task call stack anyway. >> We already do unconditional RSB-filling-on-context-switch in many >> cases. > > I think this case is a bit too obscure and theoretical to complicate the > kernel with it. You need an unmitigated processor, a > userspace-to-userspace attack that manages to satisfy the five "exploit > composition" steps of Spectre/V2[1], and an application that has been > retpoline-mitigated. > > While RSB manipulation is almost certainly less onerous than IBPB, it's > still going to hurt context-switch rates, especially if applied > indiscriminately like this patch does. > > So, I totally agree with your analysis about the theoretical potential > for an issue, I'm just not really convinced the fix is worth it. Yes, Spectre v2 looks really hard to exploit, but this doesn't mean the kernel shouldn't do its best to mitigate it. As I wrote two messages ago, basing on the Intel guidance document you linked above as "[1]" I think that the mitigation introduced by this patch should not be done on Intel CPUs, however, since that document clearly suggests that this may not be enough to cover the issue. And I think we shouldn't give people a false sense of security. Maciej
[PATCH] x86/speculation: Fill the RSB on context switch also on non-IBPB CPUs
If we run on a CPU that does not have IBPB support RSB entries from one
userspace process can influence 'ret' target prediction in another
userspace process after a context switch.
Since it is unlikely that existing RSB entries from the previous task match
the new task call stack we can use the existing unconditional
RSB-filling-on-context-switch infrastructure to protect against such
userspace-to-userspace attacks.
This patch brings a change in behavior only for the following CPU types:
* Intel pre-Skylake CPUs without updated microcode,
* AMD Family 15h model >60h, Family 17h CPUs without updated microcode.
Other CPU types either already do the RSB filling on context switch for
other reasons or do support IBPB for more complete userspace-to-userspace
protection.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/bugs.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index bfca937bdcc3..777bae86e159 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -280,8 +280,11 @@ static void __init spectre_v2_select_mitigation(void)
/*
* If neither SMEP nor PTI are available, there is a risk of
* hitting userspace addresses in the RSB after a context switch
-* from a shallow call stack to a deeper one. To prevent this fill
-* the entire RSB, even when using IBRS.
+* from a shallow call stack to a deeper one.
+* Also, if the CPU does not have IBPB support then one userspace
+* process can influence 'ret' target prediction for another
+* userspace process.
+* To prevent this fill the entire RSB, even when using IBRS.
*
* Skylake era CPUs have a separate issue with *underflow* of the
* RSB, when they will predict 'ret' targets from the generic BTB.
@@ -290,7 +293,8 @@ static void __init spectre_v2_select_mitigation(void)
* switch is required.
*/
if ((!boot_cpu_has(X86_FEATURE_PTI) &&
-!boot_cpu_has(X86_FEATURE_SMEP)) || is_skylake_era()) {
+!boot_cpu_has(X86_FEATURE_SMEP)) ||
+!boot_cpu_has(X86_FEATURE_IBPB) || is_skylake_era()) {
setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
pr_info("Spectre v2 mitigation: Filling RSB on context
switch\n");
}
Re: [PATCH] x86/speculation: Fill the RSB on context switch also on non-IBPB CPUs
On 21.03.2018 15:05, Dave Hansen wrote: > On 03/20/2018 04:17 AM, Maciej S. Szmigiero wrote: >> If we run on a CPU that does not have IBPB support RSB entries from one >> userspace process can influence 'ret' target prediction in another >> userspace process after a context switch. >> >> Since it is unlikely that existing RSB entries from the previous task match >> the new task call stack we can use the existing unconditional >> RSB-filling-on-context-switch infrastructure to protect against such >> userspace-to-userspace attacks. >> >> This patch brings a change in behavior only for the following CPU types: >> * Intel pre-Skylake CPUs without updated microcode, > > The assumption thus far (good or bad) is that everything will get a > microcode update. I actually don't know for sure if RSB manipulation is > effective on old microcode before Skylake. I'm pretty sure it has not > been documented publicly. > > How did you decide that this is an effective mitigation? > A RSB overwrite is already being done even on pre-Skylake Intel CPUs on VMEXIT to protect the host from the guest, regardless of the microcode version. But I see that an Intel guidance document published last month about retpolines says that "RET has this [predictable speculative] behavior on all processors (...) microarchitecture codename Broadwell and earlier when updated with the latest microcode". This suggests that updated microcode may be needed for protection anyway on such CPUs - as you say. Such update (hopefully) brings IBPB support, too, so I agree that the change introduced by this patch can be skipped on Intel CPUs. Maciej
Re: [PATCH] x86/speculation: Fill the RSB on context switch also on non-IBPB CPUs
On 22.03.2018 00:30, Dave Hansen wrote: > On 03/20/2018 04:17 AM, Maciej S. Szmigiero wrote: >> Since it is unlikely that existing RSB entries from the previous task match >> the new task call stack we can use the existing unconditional >> RSB-filling-on-context-switch infrastructure to protect against such >> userspace-to-userspace attacks. >> >> This patch brings a change in behavior only for the following CPU types: >> * Intel pre-Skylake CPUs without updated microcode, >> * AMD Family 15h model >60h, Family 17h CPUs without updated microcode. >> >> Other CPU types either already do the RSB filling on context switch for >> other reasons or do support IBPB for more complete userspace-to-userspace >> protection. > > I think I misunderstood your reasoning a bit. Let me see if I can > restate the problem a bit. > > IBPB provides provides userspace-to-userspace protection because it > prevents all indirect branch predictions after the barrier from being > controlled by software executed before the barrier. We only use IBPB > for KVM and when processes clear their dumpable flag. > > You're saying that, even if we don't have IBPB, we can do *some* > userspace-to-userspace protection with RSB manipulation. The RSB > manipulation obviously only helps 'RET' instructions and not JMP/CALL, > but it does do *something* useful. > > Is that right? Yes. As far as I understand the issue this should provide a good protection for userspace processes that were recompiled with retpolines as they won't have any indirect jumps and calls. > Do you really want this behavior on all context switches? We don't do > IBPB on all context switches, only the ones where we are switching *to* > a non-dumpable process. > > Do you perhaps want to do RSB manipulation in lieu of IBPB when > switching *to* a non-dumpable process and IBPB is not available? > Is it worth differentiating such processes in this case? IBPB is supposed to be very expensive so certainly it is worthwhile to do it only for high-value processes (=non-dumpable). However, it is unlikely that existing RSB entries from the previous task match the new task call stack anyway. We already do unconditional RSB-filling-on-context-switch in many cases. Maciej
Re: x86/retpoline: Fill RSB on context switch for affected CPUs
On 12.01.2018 18:49, Woodhouse, David wrote:
> When we context switch from a shallow call stack to a deeper one, as we
> 'ret' up the deeper side we may encounter RSB entries (predictions for
> where the 'ret' goes to) which were populated in userspace. This is
> problematic if we have neither SMEP nor KPTI (the latter of which marks
> userspace pages as NX for the kernel), as malicious code in userspace
> may then be executed speculatively. So overwrite the CPU's return
> prediction stack with calls which are predicted to return to an infinite
> loop, to "capture" speculation if this happens. This is required both
> for retpoline, and also in conjunction with IBRS for !SMEP && !KPTI.
>
> On Skylake+ the problem is slightly different, and an *underflow* of the
> RSB may cause errant branch predictions to occur. So there it's not so
> much overwrite, as *filling* the RSB to attempt to prevent it getting
> empty. This is only a partial solution for Skylake+ since there are many
> other conditions which may result in the RSB becoming empty. The full
> solution on Skylake+ is to use IBRS, which will prevent the problem even
> when the RSB becomes empty. With IBRS, the RSB-stuffing will not be
> required on context switch.
>
> Signed-off-by: David Woodhouse
> Acked-by: Arjan van de Ven
> ---
(..)
> @@ -213,6 +230,23 @@ static void __init spectre_v2_select_mitigation(void)
>
> spectre_v2_enabled = mode;
> pr_info("%s\n", spectre_v2_strings[mode]);
> +
> + /*
> + * If we don't have SMEP or KPTI, then we run the risk of hitting
> + * userspace addresses in the RSB after a context switch from a
> + * shallow call stack to a deeper one. We must must fill the entire
> + * RSB to avoid that, even when using IBRS.
> + *
> + * Skylake era CPUs have a separate issue with *underflow* of the
> + * RSB, when they will predict 'ret' targets from the generic BTB.
> + * IBRS makes that safe, but we need to fill the RSB on context
> + * switch if we're using retpoline.
> + */
> + if ((!boot_cpu_has(X86_FEATURE_PTI) &&
> + !boot_cpu_has(X86_FEATURE_SMEP)) || is_skylake_era()) {
> + setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
> + pr_info("Filling RSB on context switch\n");
> + }
Shouldn't the RSB filling on context switch also be done on non-IBPB
CPUs to protect (retpolined) user space tasks from other user space
tasks?
We already issue a IBPB when switching to high-value user space tasks
to protect them from other user space tasks.
Thanks,
Maciej
Re: x86/retpoline: Fill RSB on context switch for affected CPUs
On 09.03.2018 16:14, Andi Kleen wrote: >> Shouldn't the RSB filling on context switch also be done on non-IBPB >> CPUs to protect (retpolined) user space tasks from other user space >> tasks? > > The comment is actually incorrect. There's no risk to hit user space > addresses if we have KPTI and NX (which is fairly universal). > > It's mainly needed on Skylake era CPUs. > > Should fix the comment. I'll send a patch. But what about userspace-to-userspace attacks? - the ones that IBPB on context switches currently protects against (at least for high-value, or as implemented currently, non-dumpable, processes)? If understand the issue correctly, high-value user space processes can be protected from other user space processes even on CPUs that lack IBPB as long as they are recompiled with retpolines and there is no danger of RSB entries from one process being used by another one after a context switch. For Skyklake this would not be enough, but there we'll (hopefully) have the IBPB instead. > -Andi > Maciej
[PATCH v4 02/10] x86/microcode/AMD: Check equivalence table length in the early loader
Before loading a CPU equivalence table from a microcode container file we
need to verify whether this file is actually large enough to contain the
table of a size indicated in this file.
If it is not, there is no point of continuing with loading it since
microcode patches are located after the equivalence table anyway.
This patch adds these checks to the early loader.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 35 +++
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 6a93be0f771c..138c9fb983f2 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -80,20 +80,33 @@ static u16 find_equiv_id(struct equiv_cpu_entry
*equiv_table, u32 sig)
* Returns the amount of bytes consumed while scanning. @desc contains all the
* data we're going to use in later stages of the application.
*/
-static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
+static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
{
struct equiv_cpu_entry *eq;
- ssize_t orig_size = size;
+ size_t orig_size = size;
u32 *hdr = (u32 *)ucode;
+ unsigned int cont_magic, cont_type;
+ size_t equiv_tbl_len;
u16 eq_id;
u8 *buf;
+ if (size < CONTAINER_HDR_SZ)
+ return 0;
+
+ cont_magic = hdr[0];
+ cont_type = hdr[1];
+ equiv_tbl_len = hdr[2];
+
/* Am I looking at an equivalence table header? */
- if (hdr[0] != UCODE_MAGIC ||
- hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
- hdr[2] == 0)
+ if (cont_magic != UCODE_MAGIC ||
+ cont_type != UCODE_EQUIV_CPU_TABLE_TYPE ||
+ equiv_tbl_len == 0)
return CONTAINER_HDR_SZ;
+ if (equiv_tbl_len < sizeof(*eq) ||
+ size - CONTAINER_HDR_SZ < equiv_tbl_len)
+ return size;
+
buf = ucode;
eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
@@ -101,8 +114,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
/* Find the equivalence ID of our CPU in this table: */
eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
- buf += hdr[2] + CONTAINER_HDR_SZ;
- size -= hdr[2] + CONTAINER_HDR_SZ;
+ buf += equiv_tbl_len + CONTAINER_HDR_SZ;
+ size -= equiv_tbl_len + CONTAINER_HDR_SZ;
/*
* Scan through the rest of the container to find where it ends. We do
@@ -159,15 +172,13 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
*/
static void scan_containers(u8 *ucode, size_t size, struct cont_desc *desc)
{
- ssize_t rem = size;
-
- while (rem >= 0) {
- ssize_t s = parse_container(ucode, rem, desc);
+ while (size > 0) {
+ size_t s = parse_container(ucode, size, desc);
if (!s)
return;
ucode += s;
- rem -= s;
+ size -= s;
}
}
[PATCH v4 00/10] x86/microcode/AMD: Check microcode file sanity before loading it
Currently, it is very easy to make the AMD microcode update driver crash
or spin on a malformed microcode container file since it does very little
consistency checking on data loaded from such file.
This series introduces various checks, mostly on length-type fields,
so all corrupted microcode container files are (hopefully) correctly
rejected instead.
This largely matches what the Intel microcode update driver already does.
It also tries to make the behavior consistent between the early and late
loaders.
Please note that this isn't about verifying the actual microcode update,
that is, the blob that gets sent to the CPU as the new microcode.
Such verification is (hopefully) done by the CPU itself.
It is about verifying a driver-specific container file that includes
many microcode updates for different CPUs of a particular CPU family,
along with metadata that helps the driver select the right microcode
update to actually send to the CPU.
There are purposely-corrupted test files available at
https://pastebin.com/XkKUSmMp
One has to enable KASAN in the kernel config and rename a particular
test file to name appropriate to the running CPU family to test its
loading.
Changes from v1:
* Capitalize a comment,
* Rename 'eqsize' and 'bufsize' to 'eq_size' and 'buf_size',
respectively,
* Attach a comment about checking the equivalence table header to its
first size check,
* Rename 'equiv{_cpu,}_table_size' to 'equiv{_cpu,}_table_entries'.
Changes from v2:
* Split the patch into separate commits,
* Remove explicit CPU equivalence table size limit,
* Make install_equiv_cpu_table() return a size_t instead of a (signed)
int so no overflow can occur there,
* Automatically compute the PATCH_MAX_SIZE macro and use it for checking
a patch size,
* Make the late loader behavior with respect to late parse failures
consistent with what the early loader does.
Changes from v3:
* Capitalize patch subject names,
* Add a comment describing why we subtract SECTION_HDR_SIZE from a file
leftover length before calling verify_patch_size(),
* Don't break a long line containing the above subtraction,
* Move a comment in parse_container() back to where it was in the original
patch version,
* Add special local vars for container header fields in parse_container(),
* Return the remaining blob size from parse_container() if the equivalence
table is truncated,
* Split the equivalence table size checks into two patches: one for the
early loader and one for the late loader,
* Rename an equivalence table length variable in install_equiv_cpu_table()
for consistency with a similar one in parse_container(),
* Rephrase the error messages in install_equiv_cpu_table() to be more
descriptive and merge two tests there so they print the same message,
* Change install_equiv_cpu_table() to return an unsigned int while moving
the job of adding the container header length to this value to its caller,
* Drop automatic computation of the PATCH_MAX_SIZE macro and add a comment
reminding to do it manually instead,
* Add a local variable patch_type for better code readability in
verify_and_add_patch() function.
arch/x86/include/asm/microcode_amd.h | 1 +
arch/x86/kernel/cpu/microcode/amd.c | 179 ---
2 files changed, 127 insertions(+), 53 deletions(-)
[PATCH v4 01/10] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length
verify_patch_size() function verifies whether the microcode container file
remaining size is large enough to contain a patch of the indicated size.
However, the section header length is not included in this indicated size
but it is present in the leftover file length so it should be subtracted
from the leftover file length before passing this value to
verify_patch_size().
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index a998e1a7d46f..6a93be0f771c 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -613,7 +613,16 @@ static int verify_and_add_patch(u8 family, u8 *fw,
unsigned int leftover)
return crnt_size;
}
- ret = verify_patch_size(family, patch_size, leftover);
+ /*
+* verify_patch_size() checks whether the passed remaining file size
+* is large enough to contain a patch of the indicated size (and also
+* whether this size does not exceed the family maximum).
+*
+* The section header length is not included in this indicated size
+* but it is present in the leftover file length so we need to subtract
+* it before passing this value to that function.
+*/
+ ret = verify_patch_size(family, patch_size, leftover -
SECTION_HDR_SIZE);
if (!ret) {
pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
return crnt_size;
[PATCH v4 04/10] x86/microcode/AMD: install_equiv_cpu_table() should not return a signed int
The maximum possible value returned by install_equiv_cpu_table() is
UINT_MAX (on a 64-bit kernel).
This is more than (signed) int type currently returned by this function can
hold so this function will need to return an unsigned int instead.
In order to avoid an overflow of this type on a 64-bit kernel we'll need to
also modify this function to return only the CPU equivalence table length
(without the container header length), leaving its single caller
(__load_microcode_amd()) the job of adding the container header length to
skip to the fist patch section.
The individual (negative) error codes returned by install_equiv_cpu_table()
are of no use anyway, since they are all normalized to UCODE_ERROR by its
caller.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 27 ---
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index ed24200cf936..8e8df37f2f1b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -551,40 +551,39 @@ static enum ucode_state apply_microcode_amd(int cpu)
return UCODE_UPDATED;
}
-static int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
+static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
{
unsigned int *ibuf = (unsigned int *)buf;
unsigned int type, equiv_tbl_len;
if (buf_size <= CONTAINER_HDR_SZ) {
pr_err("Truncated microcode container header.\n");
- return -EINVAL;
+ return 0;
}
type = ibuf[1];
if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
pr_err("Wrong microcode container equivalence table type:
%u.\n",
type);
- return -EINVAL;
+ return 0;
}
equiv_tbl_len = ibuf[2];
if (equiv_tbl_len < sizeof(struct equiv_cpu_entry) ||
buf_size - CONTAINER_HDR_SZ < equiv_tbl_len) {
pr_err("Truncated equivalence table.\n");
- return -EINVAL;
+ return 0;
}
equiv_cpu_table = vmalloc(equiv_tbl_len);
if (!equiv_cpu_table) {
pr_err("failed to allocate equivalent CPU table\n");
- return -ENOMEM;
+ return 0;
}
memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
- /* add header length */
- return equiv_tbl_len + CONTAINER_HDR_SZ;
+ return equiv_tbl_len;
}
static void free_equiv_cpu_table(void)
@@ -681,18 +680,24 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
size_t size)
{
enum ucode_state ret = UCODE_ERROR;
- unsigned int leftover;
+ size_t leftover;
u8 *fw = (u8 *)data;
int crnt_size = 0;
- int offset;
+ unsigned int offset;
offset = install_equiv_cpu_table(data, size);
- if (offset < 0) {
+ if (!offset) {
pr_err("failed to create equivalent cpu table\n");
return ret;
}
+
+ /*
+* Skip also the container header, since install_equiv_cpu_table()
+* returns just the raw equivalence table size without the header
+*/
+ fw += CONTAINER_HDR_SZ;
fw += offset;
- leftover = size - offset;
+ leftover = size - CONTAINER_HDR_SZ - offset;
if (*(u32 *)fw != UCODE_UCODE_TYPE) {
pr_err("invalid type field in container file section header\n");
[PATCH v4 03/10] x86/microcode/AMD: Check equivalence table length in the late loader
Before loading a CPU equivalence table from a microcode container file we
need to verify whether this file is actually large enough to contain the
table of a size indicated in this file.
If it is not, there is no point of continuing with loading it since
microcode patches are located after the equivalence table anyway.
This patch adds these checks to the late loader.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 32 ++--
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 138c9fb983f2..ed24200cf936 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -551,28 +551,40 @@ static enum ucode_state apply_microcode_amd(int cpu)
return UCODE_UPDATED;
}
-static int install_equiv_cpu_table(const u8 *buf)
+static int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
{
unsigned int *ibuf = (unsigned int *)buf;
- unsigned int type = ibuf[1];
- unsigned int size = ibuf[2];
+ unsigned int type, equiv_tbl_len;
- if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
- pr_err("empty section/"
- "invalid type field in container file section header\n");
+ if (buf_size <= CONTAINER_HDR_SZ) {
+ pr_err("Truncated microcode container header.\n");
return -EINVAL;
}
- equiv_cpu_table = vmalloc(size);
+ type = ibuf[1];
+ if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
+ pr_err("Wrong microcode container equivalence table type:
%u.\n",
+ type);
+ return -EINVAL;
+ }
+
+ equiv_tbl_len = ibuf[2];
+ if (equiv_tbl_len < sizeof(struct equiv_cpu_entry) ||
+ buf_size - CONTAINER_HDR_SZ < equiv_tbl_len) {
+ pr_err("Truncated equivalence table.\n");
+ return -EINVAL;
+ }
+
+ equiv_cpu_table = vmalloc(equiv_tbl_len);
if (!equiv_cpu_table) {
pr_err("failed to allocate equivalent CPU table\n");
return -ENOMEM;
}
- memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
+ memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
/* add header length */
- return size + CONTAINER_HDR_SZ;
+ return equiv_tbl_len + CONTAINER_HDR_SZ;
}
static void free_equiv_cpu_table(void)
@@ -674,7 +686,7 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
int crnt_size = 0;
int offset;
- offset = install_equiv_cpu_table(data);
+ offset = install_equiv_cpu_table(data, size);
if (offset < 0) {
pr_err("failed to create equivalent cpu table\n");
return ret;
[PATCH v4 10/10] x86/microcode/AMD: Be more tolerant of late parse failures in late loader
The early loader just ends its microcode container file processing when it
is unable to parse some patch section, but keeps the already read patches
from this file for their eventual application.
We can do the same in the late loader - we'll just return an error if we
are unable to parse any patches.
Note that we already do silently skip patches in the late loader for
smaller issues like lack of an equivalence table entry, family-size
mismatch or an unsupported chipset match type.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 6e25a63a0a3d..f9278d9a2f9b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -738,13 +738,15 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
while (leftover) {
crnt_size = verify_and_add_patch(family, fw, leftover);
if (crnt_size < 0)
- return ret;
+ break;
fw += crnt_size;
leftover -= crnt_size;
+
+ ret = UCODE_OK;
}
- return UCODE_OK;
+ return ret;
}
static enum ucode_state
[PATCH v4 08/10] x86/microcode/AMD: Check microcode container file size before accessing it
The early loader parse_container() function should check whether the
microcode container file is actually large enough to contain the patch of
an indicated size, just like the late loader does.
Also, the request_microcode_amd() function should check whether the
container file is actually large enough to contain the header magic value.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 4d2116d08754..dc5ed4971879 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -125,6 +125,9 @@ static size_t parse_container(u8 *ucode, size_t size,
struct cont_desc *desc)
struct microcode_amd *mc;
u32 patch_size;
+ if (size < SECTION_HDR_SIZE)
+ break;
+
hdr = (u32 *)buf;
if (hdr[0] != UCODE_UCODE_TYPE)
@@ -139,6 +142,10 @@ static size_t parse_container(u8 *ucode, size_t size,
struct cont_desc *desc)
buf += SECTION_HDR_SIZE;
size -= SECTION_HDR_SIZE;
+ if (size < sizeof(*mc) ||
+ size < patch_size)
+ break;
+
mc = (struct microcode_amd *)buf;
if (eq_id == mc->hdr.processor_rev_id) {
desc->psize = patch_size;
@@ -794,6 +801,10 @@ static enum ucode_state request_microcode_amd(int cpu,
struct device *device,
}
ret = UCODE_ERROR;
+ if (fw->size < sizeof(u32)) {
+ pr_err("microcode container far too short\n");
+ goto fw_release;
+ }
if (*(u32 *)fw->data != UCODE_MAGIC) {
pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
goto fw_release;
[PATCH v4 09/10] x86/microcode/AMD: Check the equivalence table size when scanning it
Currently, the code scanning the CPU equivalence table read from a
microcode container file assumes that it actually contains a terminating
zero entry.
Let's check also the size of this table to make sure that we don't read
past it in case it actually doesn't.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 33 ++---
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index dc5ed4971879..6e25a63a0a3d 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -39,6 +39,7 @@
#include
static struct equiv_cpu_entry *equiv_cpu_table;
+static unsigned int equiv_cpu_table_entries;
/*
* This points to the current valid container of microcode patches which we
will
@@ -63,12 +64,18 @@ static u8 amd_ucode_patch[PATCH_MAX_SIZE];
static const char
ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";
-static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
+static u16 find_equiv_id(const struct equiv_cpu_entry *equiv_table,
+unsigned int equiv_table_entries, u32 sig)
{
- for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
- if (sig == equiv_table->installed_cpu)
- return equiv_table->equiv_cpu;
- }
+ unsigned int i;
+
+ if (!equiv_table)
+ return 0;
+
+ for (i = 0; i < equiv_table_entries && equiv_table[i].installed_cpu;
+i++)
+ if (sig == equiv_table[i].installed_cpu)
+ return equiv_table[i].equiv_cpu;
return 0;
}
@@ -112,7 +119,8 @@ static size_t parse_container(u8 *ucode, size_t size,
struct cont_desc *desc)
eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
/* Find the equivalence ID of our CPU in this table: */
- eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
+ eq_id = find_equiv_id(eq, equiv_tbl_len / sizeof(*eq),
+ desc->cpuid_1_eax);
buf += equiv_tbl_len + CONTAINER_HDR_SZ;
size -= equiv_tbl_len + CONTAINER_HDR_SZ;
@@ -382,20 +390,21 @@ void reload_ucode_amd(void)
static u16 __find_equiv_id(unsigned int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- return find_equiv_id(equiv_cpu_table, uci->cpu_sig.sig);
+ return find_equiv_id(equiv_cpu_table, equiv_cpu_table_entries,
+uci->cpu_sig.sig);
}
static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
{
- int i = 0;
+ unsigned int i;
BUG_ON(!equiv_cpu_table);
- while (equiv_cpu_table[i].equiv_cpu != 0) {
+ for (i = 0; i < equiv_cpu_table_entries &&
+equiv_cpu_table[i].equiv_cpu != 0; i++)
if (equiv_cpu == equiv_cpu_table[i].equiv_cpu)
return equiv_cpu_table[i].installed_cpu;
- i++;
- }
+
return 0;
}
@@ -593,6 +602,7 @@ static unsigned int install_equiv_cpu_table(const u8 *buf,
size_t buf_size)
}
memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
+ equiv_cpu_table_entries = equiv_tbl_len / sizeof(struct
equiv_cpu_entry);
return equiv_tbl_len;
}
@@ -601,6 +611,7 @@ static void free_equiv_cpu_table(void)
{
vfree(equiv_cpu_table);
equiv_cpu_table = NULL;
+ equiv_cpu_table_entries = 0;
}
static void cleanup(void)
[PATCH v4 06/10] x86/microcode/AMD: Check patch size in verify_and_add_patch()
We should verify the indicated size of a patch in a microcode container
file (whether it does not exceed the PATCH_MAX_SIZE value) and also whether
this file is actually large enough to contain it in the late loader
verify_and_add_patch() function.
The early loader already does the PATCH_MAX_SIZE check in parse_container()
function.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index eba9e3c8aa17..096cb58a563f 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -473,7 +473,7 @@ static int collect_cpu_info_amd(int cpu, struct
cpu_signature *csig)
}
static unsigned int verify_patch_size(u8 family, u32 patch_size,
- unsigned int size)
+ size_t size)
{
u32 max_size;
@@ -505,7 +505,7 @@ static unsigned int verify_patch_size(u8 family, u32
patch_size,
break;
}
- if (patch_size > min_t(u32, size, max_size)) {
+ if (patch_size > min_t(size_t, size, max_size)) {
pr_err("patch size mismatch\n");
return 0;
}
@@ -609,7 +609,7 @@ static void cleanup(void)
* driver cannot continue functioning normally. In such cases, we tear
* down everything we've used up so far and exit.
*/
-static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
+static int verify_and_add_patch(u8 family, u8 *fw, size_t leftover)
{
struct microcode_header_amd *mc_hdr;
struct ucode_patch *patch;
@@ -617,7 +617,15 @@ static int verify_and_add_patch(u8 family, u8 *fw,
unsigned int leftover)
u32 proc_fam;
u16 proc_id;
+ if (leftover < SECTION_HDR_SIZE + sizeof(*mc_hdr))
+ return leftover;
+
patch_size = *(u32 *)(fw + 4);
+ if (patch_size > PATCH_MAX_SIZE) {
+ pr_err("patch size %u too large\n", patch_size);
+ return -EINVAL;
+ }
+
crnt_size = patch_size + SECTION_HDR_SIZE;
mc_hdr = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
proc_id = mc_hdr->processor_rev_id;
[PATCH v4 07/10] x86/microcode/AMD: Verify patch section type for every such section
We should check whether the patch section currently being processed is
actually a patch section for each of them (not just the first one) in the
late loader verify_and_add_patch() function, just like the early loader
already does in parse_container() function.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 096cb58a563f..4d2116d08754 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -613,13 +613,19 @@ static int verify_and_add_patch(u8 family, u8 *fw, size_t
leftover)
{
struct microcode_header_amd *mc_hdr;
struct ucode_patch *patch;
- unsigned int patch_size, crnt_size, ret;
+ unsigned int patch_type, patch_size, crnt_size, ret;
u32 proc_fam;
u16 proc_id;
if (leftover < SECTION_HDR_SIZE + sizeof(*mc_hdr))
return leftover;
+ patch_type = *(u32 *)fw;
+ if (patch_type != UCODE_UCODE_TYPE) {
+ pr_err("invalid type field in container file section header\n");
+ return -EINVAL;
+ }
+
patch_size = *(u32 *)(fw + 4);
if (patch_size > PATCH_MAX_SIZE) {
pr_err("patch size %u too large\n", patch_size);
@@ -711,12 +717,6 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
fw += offset;
leftover = size - CONTAINER_HDR_SZ - offset;
- if (*(u32 *)fw != UCODE_UCODE_TYPE) {
- pr_err("invalid type field in container file section header\n");
- free_equiv_cpu_table();
- return ret;
- }
-
while (leftover) {
crnt_size = verify_and_add_patch(family, fw, leftover);
if (crnt_size < 0)
[PATCH v4 05/10] x86/microcode/AMD: Add a reminder about PATCH_MAX_SIZE macro
The PATCH_MAX_SIZE macro should contain the maximum of all family patch
sizes.
Since these sizes are defined in an other place than this macro, let's add
a reminder to them so people will remember to verify PATCH_MAX_SIZE
correctness when modifying a family patch size or adding a new family.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/include/asm/microcode_amd.h | 1 +
arch/x86/kernel/cpu/microcode/amd.c | 4
2 files changed, 5 insertions(+)
diff --git a/arch/x86/include/asm/microcode_amd.h
b/arch/x86/include/asm/microcode_amd.h
index 209492849566..8ea477fbc65f 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -41,6 +41,7 @@ struct microcode_amd {
unsigned intmpb[0];
};
+/* Maximum patch size of all supported families */
#define PATCH_MAX_SIZE PAGE_SIZE
#ifdef CONFIG_MICROCODE_AMD
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 8e8df37f2f1b..eba9e3c8aa17 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -477,6 +477,10 @@ static unsigned int verify_patch_size(u8 family, u32
patch_size,
{
u32 max_size;
+/*
+ * If you modify these values or add a new one also check whether
+ * PATCH_MAX_SIZE in include/asm/microcode_amd.h needs updating, too.
+ */
#define F1XH_MPB_MAX_SIZE 2048
#define F14H_MPB_MAX_SIZE 1824
#define F15H_MPB_MAX_SIZE 4096
Re: [PATCH v4 00/10] x86/microcode/AMD: Check microcode file sanity before loading it
On 16.03.2018 00:23, Borislav Petkov wrote: > On Fri, Mar 16, 2018 at 12:07:34AM +0100, Maciej S. Szmigiero wrote: >> Currently, it is very easy to make the AMD microcode update driver crash >> or spin on a malformed microcode container file since it does very little >> consistency checking on data loaded from such file. > > Ok, a couple of tips for the next time: > > * please send your reworked patchset for review after review of your > current patchset has been completed. We normally send patchsets once a > week. You've sent yours two days after and that's kinda too fast. I'm > sure you can imagine reviewers have other work to do too. > > So please be patient before resending next time. > > If in doubt, the whole process is documented in Documentation/process/ - > might wanna skim through it. I thought the one week absolute minimum resend time is for the case that there are no comments to a patch, or, more generally, when there is no action and not for "normal process" respins. submitting-patches.rst says: > Once upon a time, patches used to disappear into the void without comment, > but the development process works more smoothly than that now. You should > receive comments within a week or so; if that does not happen, make sure > that you have sent your patches to the right place. Wait for a minimum of > one week before resubmitting or pinging reviewers - possibly longer during > busy times like merge windows. But I will wait a week now for respins if you say so. > * when sending your patchset, make sure all mails are sent as a reply to > the 0/N message. > > Your 0/N has: > > Message-ID: <9964a6cf-00be-78ea-cc1e-7f7062716...@maciej.szmigiero.name> > > but your 1/N has > > References: > > so something went wrong there. Normally git send-email does this > correctly. I've copied these messages to my mailbox first and sent them from there, so that must have overwritten the message IDs. Will submit them from git-send-email the next time. > Thx. Thanks, Maciej
RANDSTRUCT structs need linux/compiler_types.h (Was: [nfsd4] potentially hardware breaking regression in 4.14-rc and 4.13.11)
On 18.11.2017 06:14, Kees Cook wrote:
> On Fri, Nov 17, 2017 at 5:54 PM, Patrick McLean wrote:
>> On 2017-11-17 04:55 PM, Linus Torvalds wrote:
>>> On Fri, Nov 17, 2017 at 4:27 PM, Patrick McLean wrote:
I am still getting the crash at d9e12200852d, I figured I would
double-check the "good" and "bad" kernels before starting a full bisect.
>>>
>>> .. but without GCC_PLUGIN_RANDSTRUCT it's solid?
>>
>> Yes, without GCC_PLUGIN_RANDSTRUCT it's solid.
>
> That's strange. With d9e12200852d the shuffle_seed variables won't
> ever actually get used. (i.e. I wouldn't expect the seed to change any
> behavior.)
>
> Can you confirm with something like this:
>
>
> for (i = 0; i < 4; i++) {
> seed[i] = shuffle_seed[i];
>
>
> You should see no reports of "Shuffling struct ..."
>
> And if it reports nothing, and you're on d9e12200852d, can you confirm
> that switching to a "good" seed fixes it? (If it _does_, then I
> suspect a build artifact being left behind or something odd like
> that.)
>
>>> Kees removed even the baseline "randomize pure function pointer
>>> structures", so at that commit, nothing should be randomized.
>>>
>>> But maybe the plugin code itself ends up confusing gcc somehow?
>>>
>>> Even when it doesn't actually do that "relayout_struct()" on the
>>> structure, it always does those TYPE_ATTRIBUTES() games.
>
> FWIW, myself doing a build at d9e12200852d with and without
> GCC_PLUGIN_RANDSTRUCT _appears_ to produce identical objdump output
> where I did spot-checks.
>
I have also hit a GPF in nfsd4_encode_fattr() with RANDSTRUCT plugin
enabled.
This function is located in a fs/nfsd/nfs4xdr.c file.
The fault happened at "xdr_encode_hyper(p,
exp->ex_path.mnt->mnt_sb->s_maxbytes)"
line, namely when accessing s_maxbytes.
exp->ex_path is of type struct path that has been annotated with
__randomize_layout.
It seems to me that this annotation isn't really taken into consideration
when compiling nfs4xdr.c.
This most likely results in dereferencing a value of exp->ex_path.dentry
instead of exp->ex_path.mnt. Then some member of struct dentry is
dereferenced as struct super_block to access its s_maxbytes member which
results in an oops if it happens to be an invalid pointer (which it was
in my case).
How to reproduce the problem statically (tested on current Linus's tree
and on 4.15.4, with gcc 7.3.0):
1) Enable RANDSTRUCT plugin,
2) Use a RANDSTRUCT seed that results in shuffling of struct path,
Example: "55e5fea7ff662b333a190209ab31f35b6f0f2470f7d0e3c64430936169571106".
3) make fs/nfsd/nfs4xdr.s and save the result,
4) Insert "#include " at the top of
fs/nfsd/nfs4xdr.c as the very first include directive.
5) make fs/nfsd/nfs4xdr.s and compare the result with the one from step 3.
One can see that offsets used to access various members of struct path are
different, and also that the original file from step 3 contains an object
named "__randomize_layout".
This is caused by a fact that the current version of nfs4xdr.c includes
linux/fs_struct.h as the very first included header which then includes
linux/path.h as the very first included header, which then defines
struct path, but without including any files on its own.
This results in __randomize_layout tag at the end of struct path
definition being treated as a variable name (since linux/compiler-gcc.h
that defines it as a type attribute has not been included yet).
It looks like to me that every header file that defines a randomized
struct also has to include linux/compiler_types.h or some other file
that ultimately results in that file inclusion in order to make
the RANDSTRUCT plugin work correctly.
Maciej
Re: [PATCH v2] kconfig.h: Include compiler types to avoid missed struct attributes
On 22.02.2018 01:28, Kees Cook wrote:
> The header files for some structures could get included in such a way
> that struct attributes (specifically __randomize_layout from path.h) would
> be parsed as variable names instead of attributes. This could lead to
> some instances of a structure being unrandomized, causing nasty GPFs, etc.
>
> This patch makes sure the compiler_types.h header is included in
> kconfig.h so that we've always got types and struct attributes defined,
> since kconfig.h is included from the compiler command line.
>
> Reported-by: Patrick McLean
> Root-caused-by: Maciej S. Szmigiero
> Fixes: 3859a271a003 ("randstruct: Mark various structs for randomization")
> Suggested-by: Linus Torvalds
> Signed-off-by: Kees Cook
I can confirm that this patch fixes the original nfsd GPF issue.
Also, struct path members offsets are consistent now between nfs4xdr.s
and other files.
> ---
> Updated Maciej's tag.
Thanks.
Maciej
Re: [PATCH v2 3/5] gpio: Change ISA_BUS_API dependency to selection
Hi William,
Hi Linus,
On 22.02.2018 21:30, William Breathitt Gray wrote:
> On Thu, Feb 22, 2018 at 04:16:17PM +0100, Linus Walleij wrote:
>> On Fri, Dec 29, 2017 at 9:13 PM, William Breathitt Gray
>> wrote:
>>
>>> The ISA_BUS_API Kconfig option enables the compilation of the ISA bus
>>> driver. The ISA bus driver does not perform any hardware interaction,
>>> and is instead just a thin layer of software abstraction to eliminate
>>> boilerplate code common to ISA-style device drivers. Since ISA_BUS_API
>>> has no dependencies and does not jeopardize the integrity of the system
>>> when enabled, drivers should select it when the ISA bus driver
>>> functionality is needed.
>>>
>>> Cc: Linus Walleij
>>> Signed-off-by: William Breathitt Gray
>>
>> Patch applied to the GPIO tree for v4.17.
>>
>> Can you confirm that we don't have any dangling ISA
>> drivers not using this?
>>
>> Yours,
>> Linus Walleij
>
> Hi Linus,
>
> This patchset should cover all current mainline drivers depending on
> ISA_BUS_API.
>
Thanks for merging this series!
Note that gpio-winbond (CONFIG_GPIO_WINBOND) driver was originally
merged using ISA_BUS_API selection as done for other ISA bus gpio drivers
by this patch, then temporary switched to the previous ("depends on")
style by commit 92a8046c9d952a to fix a circular Kconfig dependency.
So now this commit (92...) should be reverted to make gpio-winbond
driver Kconfig dependency / selection consistent with the remaining ISA
bus gpio drivers.
Unfortunately, I can't test the reversion myself because it looks like
Linus didn't push his trees yet to git.kernel.org after applying this
series (or I am not looking at the right place - linusw/linux-gpio.git?).
Best regards,
Maciej
Re: [PATCH v5 3/6] x86/microcode/AMD: Check microcode container data in the early loader
On 30.04.2018 11:05, Borislav Petkov wrote:
> On Mon, Apr 23, 2018 at 11:34:08PM +0200, Maciej S. Szmigiero wrote:
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> @@ -216,29 +216,33 @@ static bool verify_patch(u8 family, const u8 *buf,
>> size_t buf_size, bool early)
>> * Returns the amount of bytes consumed while scanning. @desc contains all
>> the
>> * data we're going to use in later stages of the application.
>> */
>> -static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc
>> *desc)
>> +static size_t parse_container(u8 *ucode, size_t size, struct cont_desc
>> *desc)
>> {
>> struct equiv_cpu_entry *eq;
>> -ssize_t orig_size = size;
>> +size_t orig_size = size;
>> u32 *hdr = (u32 *)ucode;
>> +u32 equiv_tbl_len;
>> u16 eq_id;
>> u8 *buf;
>>
>> -/* Am I looking at an equivalence table header? */
>> -if (hdr[0] != UCODE_MAGIC ||
>> -hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
>> -hdr[2] == 0)
>> +if (!verify_container(ucode, size, true))
>> +return 0;
>
> return CONTAINER_HDR_SZ;
>
> We want to make some forward progress after all.
Even if the container file main magic number is wrong? In this case
parsing is terminated by returning a zero from the above function.
If we want to make the parser more resistant to garbage or
forward-compatible to containers with a different main magic number in
their headers we probably should return a length of a single byte here
instead of 12 (CONTAINER_HDR_SZ) so the parser would correctly skip
unknown data of size which isn't an integer multiple of this number.
Thanks,
Maciej
Re: [PATCH v5 2/6] x86/microcode/AMD: Add microcode container data checking functions
On 30.04.2018 11:04, Borislav Petkov wrote:
> On Mon, Apr 23, 2018 at 11:34:07PM +0200, Maciej S. Szmigiero wrote:
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> +/*
>> + * Checks whether there is a valid, non-truncated CPU equivalence table
>> + * at the beginning of a passed buffer @buf of size @size.
>> + * If @early is set this function does not print errors which makes it
>> + * usable by the early microcode loader.
>> + */
>> +static bool verify_equivalence_table(const u8 *buf, size_t buf_size, bool
>> early)
>> +{
>> +const u32 *hdr = (const u32 *)buf;
>> +u32 cont_type, equiv_tbl_len;
>> +
>> +cont_type = hdr[1];
>
> You need to check the size of buf so that there's enough buf passed in
> before you index into it like that.
These checking functions are supposed to be called in order:
first verify_container() verifies the basic container, then
verify_equivalence_table() verifies the equivalence table while not
repeating the checks that were already done by the former function.
>> +if (cont_type != UCODE_EQUIV_CPU_TABLE_TYPE) {
>> +if (!early)
>> +pr_err("Wrong microcode container equivalence table
>> type: %u.\n",
>> + cont_type);
>> +
>> +return false;
>> +}
>> +
>> +equiv_tbl_len = hdr[2];
>
> And that.
Same situation here.
>> +
>> +/*
>> + * Checks whether a microcode patch located at the beginning of a passed
>> + * buffer @buf of size @size is not too large for a particular @family
>> + * and is not truncated.
>> + * If @early is set this function does not print errors which makes it
>> + * usable by the early microcode loader.
>> + */
>> +static bool verify_patch(u8 family, const u8 *buf, size_t buf_size, bool
>> early)
>> +{
>> +const u32 *hdr = (const u32 *)buf;
>> +u32 patch_size = hdr[1];
>
> Just like in the first comment above.
>
And a similar situation here - verify_patch() does not verify things
that were already checked by verify_container() or
verify_patch_section().
Thanks,
Maciej
Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader
On 30.04.2018 11:05, Borislav Petkov wrote:
> On Mon, Apr 23, 2018 at 11:34:09PM +0200, Maciej S. Szmigiero wrote:
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> @@ -750,28 +752,20 @@ static int verify_and_add_patch(u8 family, u8 *fw,
>> unsigned int leftover)
>> return crnt_size;
>> }
>>
>> -/*
>> - * The section header length is not included in this indicated size
>> - * but is present in the leftover file length so we need to subtract
>> - * it before passing this value to the function below.
>> - */
>> -ret = verify_patch_size(family, patch_size, leftover -
>> SECTION_HDR_SIZE);
>> -if (!ret) {
>> -pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
>> +if (!verify_patch(family, fw, leftover, false))
>> return crnt_size;
>> -}
>>
>> patch = kzalloc(sizeof(*patch), GFP_KERNEL);
>> if (!patch) {
>> pr_err("Patch allocation failure.\n");
>> -return -EINVAL;
>> +return 0;
>
> So by convention returning 0 is success and negative value means error.
> I don't see the reason for changing that in the whole code.
1) -EINVAL maps to a valid return value of 4294967274 bytes.
We have a different behavior for invalid data in the container file
(including too large lengths) than for grave errors like a failed memory
allocation.
2) This function single caller (__load_microcode_amd()) normalized any
error that verify_and_add_patch() returned to UCODE_ERROR anyway,
3) The existing code uses a convention that zero return value means
'terminate processing' for the parse_container() function in the early
loader which normally returns a 'bytes consumed' value, as this function
does.
Thanks,
Maciej
Re: [PATCH v5 2/6] x86/microcode/AMD: Add microcode container data checking functions
On 01.05.2018 10:18, Borislav Petkov wrote: > On Tue, May 01, 2018 at 12:27:17AM +0200, Maciej S. Szmigiero wrote: >> These checking functions are supposed to be called in order: > > We don't do magical rules like that - you either verify fully and > correctly or you don't bother at all. And since you're adamant that this > verification needs to happen, then please do it completely. > These are internal functions to this driver, declared as "static", so there is no problem if they have additional requirements with respect to their call order. But it is, of course, possible to do these checks also in the later checking functions as you wish. Maciej
Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader
On 01.05.2018 10:43, Borislav Petkov wrote: > On Tue, May 01, 2018 at 12:27:51AM +0200, Maciej S. Szmigiero wrote: >> 1) -EINVAL maps to a valid return value of 4294967274 bytes. >> We have a different behavior for invalid data in the container file >> (including too large lengths) than for grave errors like a failed memory >> allocation. > > WTF? Could you please elaborate this comment? -EINVAL cast to unsigned int is 4294967274 and this value is also a valid count of bytes to skip that this function can return. The "grave errors" behavior comes from the existing code, a comment in code above verify_and_add_patch() says: "a grave error like a memory allocation has failed and the driver cannot continue functioning normally. In such cases, we tear down everything we've used up so far and exit." >> 2) This function single caller (__load_microcode_amd()) normalized any >> error that verify_and_add_patch() returned to UCODE_ERROR anyway, > > So? This means that there is no loss of information here. The function these three points are about (verify_and_add_patch()) is declared as "static", so it cannot be called from any other kernel code. >> 3) The existing code uses a convention that zero return value means >> 'terminate processing' for the parse_container() function in the early >> loader which normally returns a 'bytes consumed' value, as this function >> does. > > parse_container() could very well change its convention to return > negative on error and positive value if the loop is supposed to skip > bytes. > Yes, but then the problem from the point 1) above will be introduced also to parse_container(). Maciej
Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader
On 01.05.2018 22:03, Borislav Petkov wrote: > On Tue, May 01, 2018 at 06:19:56PM +0200, Maciej S. Szmigiero wrote: >> -EINVAL cast to unsigned int is 4294967274 and this value is also >> a valid count of bytes to skip that this function can return. > > And where exactly in the *old* code do we do that? > The old code returned this value as a signed int, but then any "patch_size" value (which is u32) above INT_MAX read from a section header wrapped around to a negative pseudo-error code (which likely didn't match any actual error number). Maciej
Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader
On 03.05.2018 12:01, Borislav Petkov wrote:
> On Wed, May 02, 2018 at 02:47:39AM +0200, Maciej S. Szmigiero wrote:
>> On 01.05.2018 22:03, Borislav Petkov wrote:
>>> On Tue, May 01, 2018 at 06:19:56PM +0200, Maciej S. Szmigiero wrote:
>>>> -EINVAL cast to unsigned int is 4294967274 and this value is also
>>>> a valid count of bytes to skip that this function can return.
>>>
>>> And where exactly in the *old* code do we do that?
>>
>> The old code returned this value as a signed int, but then any
>> "patch_size" value (which is u32) above INT_MAX read from a section header
>> wrapped around to a negative pseudo-error code (which likely didn't match
>> any actual error number).
>
> Lemme repeat my question: *where* *exactly* in the old code do we do that?
>
> Feel free to paste snippets to show what you mean.
>
>From verify_and_add_patch():
> static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
> {
> struct microcode_header_amd *mc_hdr;
> struct ucode_patch *patch;
> unsigned int patch_size, crnt_size, ret;
> u32 proc_fam;
> u16 proc_id;
>
> patch_size = *(u32 *)(fw + 4);
Here we read a u32 (= unsigned int) value from a section header
and store it into an unsigned int variable.
> crnt_size = patch_size + SECTION_HDR_SIZE;
Here we add 8 (SECTION_HDR_SIZE) to this value and once again store it
into an unsigned int variable.
> mc_hdr = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
> proc_id = mc_hdr->processor_rev_id;
>
> proc_fam = find_cpu_family_by_equiv_cpu(proc_id);
> if (!proc_fam) {
> pr_err("No patch family for equiv ID: 0x%04x\n", proc_id);
> return crnt_size;
Here we return this variable, implicitly converting it into a
(signed) int.
Any value above INT_MAX will wrap around to a negative pseudo-error
code (which might not match any actual error number).
Maciej
[PATCH v2][RESEND] X.509: unpack RSA signatureValue field from BIT STRING
The signatureValue field of a X.509 certificate is encoded as a BIT STRING.
For RSA signatures this BIT STRING is of so-called primitive subtype, which
contains a u8 prefix indicating a count of unused bits in the encoding.
We have to strip this prefix from signature data, just as we already do for
key data in x509_extract_key_data() function.
This wasn't noticed earlier because this prefix byte is zero for RSA key
sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero
prefixes has no bearing on its value.
The signature length, however was incorrect, which is a problem for RSA
implementations that need it to be exactly correct (like AMD CCP).
Signed-off-by: Maciej S. Szmigiero
Fixes: c26fd69fa009 ("X.509: Add a crypto key parser for binary (DER) X.509
certificates")
Cc: sta...@vger.kernel.org
---
This is a resend of a patch that was previously submitted in one series
with CCP driver changes since this particular patch should go through
the security (rather than crypto) tree.
Changes from v1: Change '!' to '== 0'.
crypto/asymmetric_keys/x509_cert_parser.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
b/crypto/asymmetric_keys/x509_cert_parser.c
index 7d81e6bb461a..b6cabac4b62b 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -249,6 +249,15 @@ int x509_note_signature(void *context, size_t hdrlen,
return -EINVAL;
}
+ if (strcmp(ctx->cert->sig->pkey_algo, "rsa") == 0) {
+ /* Discard the BIT STRING metadata */
+ if (vlen < 1 || *(const u8 *)value != 0)
+ return -EBADMSG;
+
+ value++;
+ vlen--;
+ }
+
ctx->cert->raw_sig = value;
ctx->cert->raw_sig_size = vlen;
return 0;
[PATCH v6 3/8] x86/microcode/AMD: Check microcode container data in the early loader
Convert the early loader in the AMD microcode update driver to use the
container data checking functions introduced by the previous commit.
We have to be careful to call these functions with 'early' parameter set,
so they won't try to print errors as the early loader runs too early for
printk()-style functions to work.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 59 -
1 file changed, 33 insertions(+), 26 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index f9485ff7183c..f4c7479a961c 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -224,29 +224,36 @@ static bool verify_patch(u8 family, const u8 *buf, size_t
buf_size, bool early)
* Returns the amount of bytes consumed while scanning. @desc contains all the
* data we're going to use in later stages of the application.
*/
-static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
+static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
{
struct equiv_cpu_entry *eq;
- ssize_t orig_size = size;
+ size_t orig_size = size;
u32 *hdr = (u32 *)ucode;
+ u32 equiv_tbl_len;
u16 eq_id;
u8 *buf;
- /* Am I looking at an equivalence table header? */
- if (hdr[0] != UCODE_MAGIC ||
- hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
- hdr[2] == 0)
- return CONTAINER_HDR_SZ;
+ /*
+* Skip one byte when a container cannot be parsed successfully
+* so the parser will correctly skip unknown data of any size until
+* it hopefully arrives at something that it is able to recognize.
+*/
+ if (!verify_container(ucode, size, true) ||
+ !verify_equivalence_table(ucode, size, true))
+ return 1;
buf = ucode;
+ equiv_tbl_len = hdr[2];
eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
/* Find the equivalence ID of our CPU in this table: */
eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
- buf += hdr[2] + CONTAINER_HDR_SZ;
- size -= hdr[2] + CONTAINER_HDR_SZ;
+ buf += CONTAINER_HDR_SZ;
+ buf += equiv_tbl_len;
+ size -= CONTAINER_HDR_SZ;
+ size -= equiv_tbl_len;
/*
* Scan through the rest of the container to find where it ends. We do
@@ -258,25 +265,27 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
hdr = (u32 *)buf;
- if (hdr[0] != UCODE_UCODE_TYPE)
+ if (!verify_patch_section(buf, size, true))
break;
- /* Sanity-check patch size. */
patch_size = hdr[1];
- if (patch_size > PATCH_MAX_SIZE)
- break;
- /* Skip patch section header: */
- buf += SECTION_HDR_SIZE;
- size -= SECTION_HDR_SIZE;
+ mc = (struct microcode_amd *)(buf + SECTION_HDR_SIZE);
+ if (eq_id != mc->hdr.processor_rev_id)
+ goto next_patch;
- mc = (struct microcode_amd *)buf;
- if (eq_id == mc->hdr.processor_rev_id) {
- desc->psize = patch_size;
- desc->mc = mc;
- }
+ if (!verify_patch(x86_family(desc->cpuid_1_eax), buf, size,
+ true))
+ goto next_patch;
+
+ /* We have found a matching patch! */
+ desc->psize = patch_size;
+ desc->mc = mc;
+next_patch:
+ buf += SECTION_HDR_SIZE;
buf += patch_size;
+ size -= SECTION_HDR_SIZE;
size -= patch_size;
}
@@ -303,15 +312,13 @@ static ssize_t parse_container(u8 *ucode, ssize_t size,
struct cont_desc *desc)
*/
static void scan_containers(u8 *ucode, size_t size, struct cont_desc *desc)
{
- ssize_t rem = size;
-
- while (rem >= 0) {
- ssize_t s = parse_container(ucode, rem, desc);
+ while (size > 0) {
+ size_t s = parse_container(ucode, size, desc);
if (!s)
return;
ucode += s;
- rem -= s;
+ size -= s;
}
}
[PATCH v6 2/8] x86/microcode/AMD: Add microcode container data checking functions
Add verify_container(), verify_equivalence_table(), verify_patch_section()
and verify_patch() functions to the AMD microcode update driver.
These functions check whether a passed buffer contains the relevant
structure, whether it isn't truncated and (for actual microcode patches)
whether the size of a patch is not too large for a particular CPU family.
By adding these checks as separate functions the actual microcode loading
code won't get interspersed with a lot of checks and so will be more
readable.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 148 +++-
1 file changed, 145 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index dc8ea9a9d962..f9485ff7183c 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -73,6 +73,150 @@ static u16 find_equiv_id(struct equiv_cpu_entry
*equiv_table, u32 sig)
return 0;
}
+/*
+ * Checks whether there is a valid microcode container file at the beginning
+ * of a passed buffer @buf of size @size.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_container(const u8 *buf, size_t buf_size, bool early)
+{
+ u32 cont_magic;
+
+ if (buf_size <= CONTAINER_HDR_SZ) {
+ if (!early)
+ pr_err("Truncated microcode container header.\n");
+
+ return false;
+ }
+
+ cont_magic = *(const u32 *)buf;
+ if (cont_magic != UCODE_MAGIC) {
+ if (!early)
+ pr_err("Invalid magic value (0x%08x).\n", cont_magic);
+
+ return false;
+ }
+
+ return true;
+}
+
+/*
+ * Checks whether there is a valid, non-truncated CPU equivalence table
+ * at the beginning of a passed buffer @buf of size @size.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_equivalence_table(const u8 *buf, size_t buf_size, bool
early)
+{
+ const u32 *hdr = (const u32 *)buf;
+ u32 cont_type, equiv_tbl_len;
+
+ if (!verify_container(buf, buf_size, early))
+ return false;
+
+ cont_type = hdr[1];
+ if (cont_type != UCODE_EQUIV_CPU_TABLE_TYPE) {
+ if (!early)
+ pr_err("Wrong microcode container equivalence table
type: %u.\n",
+ cont_type);
+
+ return false;
+ }
+
+ equiv_tbl_len = hdr[2];
+ if (equiv_tbl_len < sizeof(struct equiv_cpu_entry) ||
+ buf_size - CONTAINER_HDR_SZ < equiv_tbl_len) {
+ if (!early)
+ pr_err("Truncated equivalence table.\n");
+
+ return false;
+ }
+
+ return true;
+}
+
+/*
+ * Checks whether there is a valid, non-truncated microcode patch section
+ * at the beginning of a passed buffer @buf of size @size.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_patch_section(const u8 *buf, size_t buf_size, bool early)
+{
+ const u32 *hdr = (const u32 *)buf;
+ u32 patch_type, patch_size;
+
+ if (buf_size < SECTION_HDR_SIZE) {
+ if (!early)
+ pr_err("Truncated patch section.\n");
+
+ return false;
+ }
+
+ patch_type = hdr[0];
+ patch_size = hdr[1];
+
+ if (patch_type != UCODE_UCODE_TYPE) {
+ if (!early)
+ pr_err("Invalid type field (%u) in container file
section header.\n",
+ patch_type);
+
+ return false;
+ }
+
+ if (patch_size < sizeof(struct microcode_header_amd)) {
+ if (!early)
+ pr_err("Patch of size %u too short.\n", patch_size);
+
+ return false;
+ }
+
+ if (buf_size - SECTION_HDR_SIZE < patch_size) {
+ if (!early)
+ pr_err("Patch of size %u truncated.\n", patch_size);
+
+ return false;
+ }
+
+ return true;
+}
+
+static unsigned int verify_patch_size(u8 family, u32 patch_size,
+ unsigned int size);
+
+/*
+ * Checks whether a microcode patch located at the beginning of a passed
+ * buffer @buf of size @size is not too large for a particular @family
+ * and is not truncated.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_patch(u8 family, const u8 *buf, size_t buf_size, bool early)
+{
+ const u32 *hdr = (const u32 *)buf;
+ u32 patch_size;
+
+ if (!veri
[PATCH v6 0/8] x86/microcode/AMD: Check microcode file sanity before loading it
Currently, it is very easy to make the AMD microcode update driver crash
or spin on a malformed microcode container file since it does very little
consistency checking on data loaded from such file.
This series introduces various checks, mostly on length-type fields,
so all corrupted microcode container files are (hopefully) correctly
rejected instead.
This largely matches what the Intel microcode update driver already does.
It also tries to make the behavior consistent between the early and late
loaders.
Please note that this isn't about verifying the actual microcode update,
that is, the blob that gets sent to the CPU as the new microcode.
Such verification is (hopefully) done by the CPU itself.
It is about verifying a driver-specific container file that includes
many microcode updates for different CPUs of a particular CPU family,
along with metadata that helps the driver select the right microcode
update to actually send to the CPU.
There are purposely-corrupted test files available at
https://pastebin.com/XkKUSmMp
One has to enable KASAN in the kernel config and rename a particular
test file to name appropriate to the running CPU family to test its
loading.
Changes from v1:
* Capitalize a comment,
* Rename 'eqsize' and 'bufsize' to 'eq_size' and 'buf_size',
respectively,
* Attach a comment about checking the equivalence table header to its
first size check,
* Rename 'equiv{_cpu,}_table_size' to 'equiv{_cpu,}_table_entries'.
Changes from v2:
* Split the patch into separate commits,
* Remove explicit CPU equivalence table size limit,
* Make install_equiv_cpu_table() return a size_t instead of a (signed)
int so no overflow can occur there,
* Automatically compute the PATCH_MAX_SIZE macro and use it for checking
a patch size,
* Make the late loader behavior with respect to late parse failures
consistent with what the early loader does.
Changes from v3:
* Capitalize patch subject names,
* Add a comment describing why we subtract SECTION_HDR_SIZE from a file
leftover length before calling verify_patch_size(),
* Don't break a long line containing the above subtraction,
* Move a comment in parse_container() back to where it was in the original
patch version,
* Add special local vars for container header fields in parse_container(),
* Return the remaining blob size from parse_container() if the equivalence
table is truncated,
* Split the equivalence table size checks into two patches: one for the
early loader and one for the late loader,
* Rename an equivalence table length variable in install_equiv_cpu_table()
for consistency with a similar one in parse_container(),
* Rephrase the error messages in install_equiv_cpu_table() to be more
descriptive and merge two tests there so they print the same message,
* Change install_equiv_cpu_table() to return an unsigned int while moving
the job of adding the container header length to this value to its caller,
* Drop automatic computation of the PATCH_MAX_SIZE macro and add a comment
reminding to do it manually instead,
* Add a local variable patch_type for better code readability in
verify_and_add_patch() function.
Changes from v4:
* Update the first patch in series with Borislav's version,
* Use u32-type variables for microcode container header fields,
* Drop the check for zero-length CPU equivalence table,
* Leave size variables in verify_patch_size() as unsigned ints,
* Rewrite the series to use common microcode container data checking
functions in both the early and the late loader so their code won't get
interspersed with a lot of checks and so will be more readable.
Changes from v5:
* Remove "This commit" or "This patch" prefix from commit messages,
* Make sure that every checking function verifies presence of any data
it accesses so these functions don't have an implicit call order
requirement,
* Integrate verify_patch_size() into verify_patch() after the late loader
is converted to no longer call verify_patch_size() directly,
* Make a patch matching check in the early loader more readable,
* Skip just one byte in the early loader when a container cannot be parsed
successfully so the parser will correctly skip unknown data of any size,
* Move assignment of a pointer to CPU equivalence table header variable
past this table check in install_equiv_cpu_table(),
* Split returned status value from returned length of data to skip in
verify_and_add_patch() so we keep the common convention that a function
zero return value means success while a negative value means an error,
* Don't introduce a new global variable for holding the CPU equivalence
table size, add a terminating zero entry to this table explicitly instead
to prevent scanning past its valid data.
arch/x86/include/asm/microcode_amd.h | 1 +
arch/x86/kernel/cpu/microcode/amd.c | 403 +++
2 files changed, 285 insertions(+), 119 deletions(-)
[PATCH v6 4/8] x86/microcode/AMD: Split status from data to skip in verify_and_add_patch()
verify_and_add_patch() returned a single "int" value which encoded both
this function error status and also a length of microcode container data to
skip.
Unfortunately, ranges of these two values collide: the length of data to
skip can be any value between 1 and UINT_MAX, so, for example, error status
of -EINVAL maps to a valid return value of 4294967274 bytes.
That's why these two values need to be split.
Let's keep the common convention that a function zero return value means
success while a negative value means an error while moving the returned
length of microcode container data to skip to a separate output parameter.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 33 +++--
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index f4c7479a961c..f8bd74341ed8 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -730,40 +730,41 @@ static void cleanup(void)
}
/*
- * We return the current size even if some of the checks failed so that
- * we can skip over the next patch. If we return a negative value, we
- * signal a grave error like a memory allocation has failed and the
- * driver cannot continue functioning normally. In such cases, we tear
- * down everything we've used up so far and exit.
+ * We return zero (success) and the current patch data size in @crnt_size
+ * even if some of the checks failed so that we can skip over the next patch.
+ * If we return a negative value, we signal a grave error like a memory
+ * allocation has failed and the driver cannot continue functioning normally.
+ * In such cases, we tear down everything we've used up so far and exit.
*/
-static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
+static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
+ unsigned int *crnt_size)
{
struct microcode_header_amd *mc_hdr;
struct ucode_patch *patch;
- unsigned int patch_size, crnt_size, ret;
+ unsigned int patch_size, ret;
u32 proc_fam;
u16 proc_id;
patch_size = *(u32 *)(fw + 4);
- crnt_size = patch_size + SECTION_HDR_SIZE;
+ *crnt_size = patch_size + SECTION_HDR_SIZE;
mc_hdr = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
proc_id = mc_hdr->processor_rev_id;
proc_fam = find_cpu_family_by_equiv_cpu(proc_id);
if (!proc_fam) {
pr_err("No patch family for equiv ID: 0x%04x\n", proc_id);
- return crnt_size;
+ return 0;
}
/* check if patch is for the current family */
proc_fam = ((proc_fam >> 8) & 0xf) + ((proc_fam >> 20) & 0xff);
if (proc_fam != family)
- return crnt_size;
+ return 0;
if (mc_hdr->nb_dev_id || mc_hdr->sb_dev_id) {
pr_err("Patch-ID 0x%08x: chipset-specific code unsupported.\n",
mc_hdr->patch_id);
- return crnt_size;
+ return 0;
}
/*
@@ -774,7 +775,7 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned
int leftover)
ret = verify_patch_size(family, patch_size, leftover -
SECTION_HDR_SIZE);
if (!ret) {
pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
- return crnt_size;
+ return 0;
}
patch = kzalloc(sizeof(*patch), GFP_KERNEL);
@@ -800,7 +801,7 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned
int leftover)
/* ... and add to cache. */
update_cache(patch);
- return crnt_size;
+ return 0;
}
static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
@@ -809,7 +810,6 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
enum ucode_state ret = UCODE_ERROR;
unsigned int leftover;
u8 *fw = (u8 *)data;
- int crnt_size = 0;
int offset;
offset = install_equiv_cpu_table(data);
@@ -827,8 +827,9 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
}
while (leftover) {
- crnt_size = verify_and_add_patch(family, fw, leftover);
- if (crnt_size < 0)
+ unsigned int crnt_size;
+
+ if (verify_and_add_patch(family, fw, leftover, _size) < 0)
return ret;
fw += crnt_size;
[PATCH v6 1/8] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length
verify_patch_size() verifies whether the remaining size of the microcode
container file is large enough to contain a patch of the indicated size.
However, the section header length is not included in this indicated
size but it is present in the leftover file length so it should be
subtracted from the leftover file length before passing this value to
verify_patch_size().
Signed-off-by: Maciej S. Szmigiero
Link:
http://lkml.kernel.org/r/b4854b17-e3ba-54d6-488d-0e0bfffe4...@maciej.szmigiero.name
[ Split comment. ]
Signed-off-by: Borislav Petkov
---
arch/x86/kernel/cpu/microcode/amd.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 0624957aa068..dc8ea9a9d962 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -461,8 +461,12 @@ static int collect_cpu_info_amd(int cpu, struct
cpu_signature *csig)
return 0;
}
-static unsigned int verify_patch_size(u8 family, u32 patch_size,
- unsigned int size)
+/*
+ * Check whether the passed remaining file @size is large enough to contain a
+ * patch of the indicated @patch_size (and also whether this size does not
+ * exceed the per-family maximum).
+ */
+static unsigned int verify_patch_size(u8 family, u32 patch_size, unsigned int
size)
{
u32 max_size;
@@ -613,7 +617,12 @@ static int verify_and_add_patch(u8 family, u8 *fw,
unsigned int leftover)
return crnt_size;
}
- ret = verify_patch_size(family, patch_size, leftover);
+ /*
+* The section header length is not included in this indicated size
+* but is present in the leftover file length so we need to subtract
+* it before passing this value to the function below.
+*/
+ ret = verify_patch_size(family, patch_size, leftover -
SECTION_HDR_SIZE);
if (!ret) {
pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
return crnt_size;
[PATCH v6 6/8] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch()
Now that the previous commit converted the late loader to do a full patch
checking via verify_patch() instead of calling verify_patch_size() directly
we can integrate the later function into the former because it was its only
user.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 72 +++--
1 file changed, 27 insertions(+), 45 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 3e10a5920f58..764dac82c03f 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -182,9 +182,6 @@ static bool verify_patch_section(const u8 *buf, size_t
buf_size, bool early)
return true;
}
-static unsigned int verify_patch_size(u8 family, u32 patch_size,
- unsigned int size);
-
/*
* Checks whether a microcode patch located at the beginning of a passed
* buffer @buf of size @size is not too large for a particular @family
@@ -195,19 +192,43 @@ static unsigned int verify_patch_size(u8 family, u32
patch_size,
static bool verify_patch(u8 family, const u8 *buf, size_t buf_size, bool early)
{
const u32 *hdr = (const u32 *)buf;
- u32 patch_size;
+ u32 patch_size, max_size;
if (!verify_patch_section(buf, buf_size, early))
return false;
patch_size = hdr[1];
+#define F1XH_MPB_MAX_SIZE 2048
+#define F14H_MPB_MAX_SIZE 1824
+#define F15H_MPB_MAX_SIZE 4096
+#define F16H_MPB_MAX_SIZE 3458
+#define F17H_MPB_MAX_SIZE 3200
+
+ switch (family) {
+ case 0x14:
+ max_size = F14H_MPB_MAX_SIZE;
+ break;
+ case 0x15:
+ max_size = F15H_MPB_MAX_SIZE;
+ break;
+ case 0x16:
+ max_size = F16H_MPB_MAX_SIZE;
+ break;
+ case 0x17:
+ max_size = F17H_MPB_MAX_SIZE;
+ break;
+ default:
+ max_size = F1XH_MPB_MAX_SIZE;
+ break;
+ }
+
/*
* The section header length is not included in this indicated size
* but is present in the leftover file length so we need to subtract
-* it before passing this value to the function below.
+* it from the leftover file length.
*/
- if (!verify_patch_size(family, patch_size, buf_size -
SECTION_HDR_SIZE)) {
+ if (patch_size > min_t(u32, max_size, buf_size - SECTION_HDR_SIZE)) {
if (!early)
pr_err("Patch of size %u too large.\n", patch_size);
@@ -612,45 +633,6 @@ static int collect_cpu_info_amd(int cpu, struct
cpu_signature *csig)
return 0;
}
-/*
- * Check whether the passed remaining file @size is large enough to contain a
- * patch of the indicated @patch_size (and also whether this size does not
- * exceed the per-family maximum).
- */
-static unsigned int verify_patch_size(u8 family, u32 patch_size, unsigned int
size)
-{
- u32 max_size;
-
-#define F1XH_MPB_MAX_SIZE 2048
-#define F14H_MPB_MAX_SIZE 1824
-#define F15H_MPB_MAX_SIZE 4096
-#define F16H_MPB_MAX_SIZE 3458
-#define F17H_MPB_MAX_SIZE 3200
-
- switch (family) {
- case 0x14:
- max_size = F14H_MPB_MAX_SIZE;
- break;
- case 0x15:
- max_size = F15H_MPB_MAX_SIZE;
- break;
- case 0x16:
- max_size = F16H_MPB_MAX_SIZE;
- break;
- case 0x17:
- max_size = F17H_MPB_MAX_SIZE;
- break;
- default:
- max_size = F1XH_MPB_MAX_SIZE;
- break;
- }
-
- if (patch_size > min_t(u32, size, max_size))
- return 0;
-
- return patch_size;
-}
-
static enum ucode_state apply_microcode_amd(int cpu)
{
struct cpuinfo_x86 *c = _data(cpu);
[PATCH v6 5/8] x86/microcode/AMD: Check microcode container data in the late loader
Convert the late loader in the AMD microcode update driver to use newly
introduced microcode container data checking functions as it was previously
done for the early loader.
Signed-off-by: Maciej S. Szmigiero
---
arch/x86/kernel/cpu/microcode/amd.c | 70 +
1 file changed, 32 insertions(+), 38 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index f8bd74341ed8..3e10a5920f58 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -693,28 +693,26 @@ static enum ucode_state apply_microcode_amd(int cpu)
return UCODE_UPDATED;
}
-static int install_equiv_cpu_table(const u8 *buf)
+static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
{
- unsigned int *ibuf = (unsigned int *)buf;
- unsigned int type = ibuf[1];
- unsigned int size = ibuf[2];
+ const u32 *hdr;
+ u32 equiv_tbl_len;
- if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
- pr_err("empty section/"
- "invalid type field in container file section header\n");
- return -EINVAL;
- }
+ if (!verify_equivalence_table(buf, buf_size, false))
+ return 0;
+
+ hdr = (const u32 *)buf;
+ equiv_tbl_len = hdr[2];
- equiv_cpu_table = vmalloc(size);
+ equiv_cpu_table = vmalloc(equiv_tbl_len);
if (!equiv_cpu_table) {
pr_err("failed to allocate equivalent CPU table\n");
- return -ENOMEM;
+ return 0;
}
- memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
+ memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
- /* add header length */
- return size + CONTAINER_HDR_SZ;
+ return equiv_tbl_len;
}
static void free_equiv_cpu_table(void)
@@ -739,13 +737,19 @@ static void cleanup(void)
static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
unsigned int *crnt_size)
{
+ u32 *hdr = (u32 *)fw;
struct microcode_header_amd *mc_hdr;
struct ucode_patch *patch;
- unsigned int patch_size, ret;
+ u32 patch_size;
u32 proc_fam;
u16 proc_id;
- patch_size = *(u32 *)(fw + 4);
+ if (!verify_patch_section(fw, leftover, false)) {
+ *crnt_size = leftover;
+ return 0;
+ }
+
+ patch_size = hdr[1];
*crnt_size = patch_size + SECTION_HDR_SIZE;
mc_hdr = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
proc_id = mc_hdr->processor_rev_id;
@@ -767,16 +771,8 @@ static int verify_and_add_patch(u8 family, u8 *fw,
unsigned int leftover,
return 0;
}
- /*
-* The section header length is not included in this indicated size
-* but is present in the leftover file length so we need to subtract
-* it before passing this value to the function below.
-*/
- ret = verify_patch_size(family, patch_size, leftover -
SECTION_HDR_SIZE);
- if (!ret) {
- pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
+ if (!verify_patch(family, fw, leftover, false))
return 0;
- }
patch = kzalloc(sizeof(*patch), GFP_KERNEL);
if (!patch) {
@@ -810,21 +806,21 @@ static enum ucode_state __load_microcode_amd(u8 family,
const u8 *data,
enum ucode_state ret = UCODE_ERROR;
unsigned int leftover;
u8 *fw = (u8 *)data;
- int offset;
+ unsigned int offset;
- offset = install_equiv_cpu_table(data);
- if (offset < 0) {
+ offset = install_equiv_cpu_table(data, size);
+ if (!offset) {
pr_err("failed to create equivalent cpu table\n");
return ret;
}
- fw += offset;
- leftover = size - offset;
- if (*(u32 *)fw != UCODE_UCODE_TYPE) {
- pr_err("invalid type field in container file section header\n");
- free_equiv_cpu_table();
- return ret;
- }
+ /*
+* Skip also the container header, since install_equiv_cpu_table()
+* returns just the raw equivalence table size without the header.
+*/
+ fw += CONTAINER_HDR_SZ;
+ fw += offset;
+ leftover = size - CONTAINER_HDR_SZ - offset;
while (leftover) {
unsigned int crnt_size;
@@ -912,10 +908,8 @@ static enum ucode_state request_microcode_amd(int cpu,
struct device *device,
}
ret = UCODE_ERROR;
- if (*(u32 *)fw->data != UCODE_MAGIC) {
- pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
+ if (!verify_container(fw->data, fw->size, false))
goto fw_release;
- }
ret = load_microcode_amd(bsp, c->x86, fw->data, fw->size);
