Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 1 | .. SPDX-License-Identifier: GPL-2.0 |
| 2 | |
Federico Vaga | 9834857 | 2019-02-24 21:05:27 +0100 | [diff] [blame] | 3 | .. _deprecated: |
| 4 | |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 5 | ===================================================================== |
| 6 | Deprecated Interfaces, Language Features, Attributes, and Conventions |
| 7 | ===================================================================== |
| 8 | |
| 9 | In a perfect world, it would be possible to convert all instances of |
| 10 | some deprecated API into the new API and entirely remove the old API in |
| 11 | a single development cycle. However, due to the size of the kernel, the |
| 12 | maintainership hierarchy, and timing, it's not always feasible to do these |
| 13 | kinds of conversions at once. This means that new instances may sneak into |
| 14 | the kernel while old ones are being removed, only making the amount of |
| 15 | work to remove the API grow. In order to educate developers about what |
| 16 | has been deprecated and why, this list has been created as a place to |
| 17 | point when uses of deprecated things are proposed for inclusion in the |
| 18 | kernel. |
| 19 | |
| 20 | __deprecated |
| 21 | ------------ |
| 22 | While this attribute does visually mark an interface as deprecated, |
| 23 | it `does not produce warnings during builds any more |
| 24 | <https://git.kernel.org/linus/771c035372a036f83353eef46dbb829780330234>`_ |
| 25 | because one of the standing goals of the kernel is to build without |
| 26 | warnings and no one was actually doing anything to remove these deprecated |
| 27 | interfaces. While using `__deprecated` is nice to note an old API in |
| 28 | a header file, it isn't the full solution. Such interfaces must either |
| 29 | be fully removed from the kernel, or added to this file to discourage |
| 30 | others from using them in the future. |
| 31 | |
| 32 | open-coded arithmetic in allocator arguments |
| 33 | -------------------------------------------- |
| 34 | Dynamic size calculations (especially multiplication) should not be |
| 35 | performed in memory allocator (or similar) function arguments due to the |
| 36 | risk of them overflowing. This could lead to values wrapping around and a |
| 37 | smaller allocation being made than the caller was expecting. Using those |
| 38 | allocations could lead to linear overflows of heap memory and other |
| 39 | misbehaviors. (One exception to this is literal values where the compiler |
| 40 | can warn if they might overflow. Though using literals for arguments as |
| 41 | suggested below is also harmless.) |
| 42 | |
| 43 | For example, do not use ``count * size`` as an argument, as in:: |
| 44 | |
| 45 | foo = kmalloc(count * size, GFP_KERNEL); |
| 46 | |
| 47 | Instead, the 2-factor form of the allocator should be used:: |
| 48 | |
| 49 | foo = kmalloc_array(count, size, GFP_KERNEL); |
| 50 | |
| 51 | If no 2-factor form is available, the saturate-on-overflow helpers should |
| 52 | be used:: |
| 53 | |
| 54 | bar = vmalloc(array_size(count, size)); |
| 55 | |
| 56 | Another common case to avoid is calculating the size of a structure with |
| 57 | a trailing array of others structures, as in:: |
| 58 | |
| 59 | header = kzalloc(sizeof(*header) + count * sizeof(*header->item), |
| 60 | GFP_KERNEL); |
| 61 | |
| 62 | Instead, use the helper:: |
| 63 | |
| 64 | header = kzalloc(struct_size(header, item, count), GFP_KERNEL); |
| 65 | |
| 66 | See :c:func:`array_size`, :c:func:`array3_size`, and :c:func:`struct_size`, |
| 67 | for more details as well as the related :c:func:`check_add_overflow` and |
| 68 | :c:func:`check_mul_overflow` family of functions. |
| 69 | |
| 70 | simple_strtol(), simple_strtoll(), simple_strtoul(), simple_strtoull() |
| 71 | ---------------------------------------------------------------------- |
| 72 | The :c:func:`simple_strtol`, :c:func:`simple_strtoll`, |
| 73 | :c:func:`simple_strtoul`, and :c:func:`simple_strtoull` functions |
| 74 | explicitly ignore overflows, which may lead to unexpected results |
| 75 | in callers. The respective :c:func:`kstrtol`, :c:func:`kstrtoll`, |
| 76 | :c:func:`kstrtoul`, and :c:func:`kstrtoull` functions tend to be the |
| 77 | correct replacements, though note that those require the string to be |
| 78 | NUL or newline terminated. |
| 79 | |
| 80 | strcpy() |
| 81 | -------- |
| 82 | :c:func:`strcpy` performs no bounds checking on the destination |
| 83 | buffer. This could result in linear overflows beyond the |
| 84 | end of the buffer, leading to all kinds of misbehaviors. While |
| 85 | `CONFIG_FORTIFY_SOURCE=y` and various compiler flags help reduce the |
| 86 | risk of using this function, there is no good reason to add new uses of |
| 87 | this function. The safe replacement is :c:func:`strscpy`. |
| 88 | |
| 89 | strncpy() on NUL-terminated strings |
| 90 | ----------------------------------- |
| 91 | Use of :c:func:`strncpy` does not guarantee that the destination buffer |
| 92 | will be NUL terminated. This can lead to various linear read overflows |
| 93 | and other misbehavior due to the missing termination. It also NUL-pads the |
| 94 | destination buffer if the source contents are shorter than the destination |
| 95 | buffer size, which may be a needless performance penalty for callers using |
| 96 | only NUL-terminated strings. The safe replacement is :c:func:`strscpy`. |
Kees Cook | 76136e0 | 2020-03-04 11:03:24 -0800 | [diff] [blame^] | 97 | (Users of :c:func:`strscpy` still needing NUL-padding should instead |
| 98 | use strscpy_pad().) |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 99 | |
| 100 | If a caller is using non-NUL-terminated strings, :c:func:`strncpy()` can |
| 101 | still be used, but destinations should be marked with the `__nonstring |
| 102 | <https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_ |
| 103 | attribute to avoid future compiler warnings. |
| 104 | |
| 105 | strlcpy() |
| 106 | --------- |
| 107 | :c:func:`strlcpy` reads the entire source buffer first, possibly exceeding |
| 108 | the given limit of bytes to copy. This is inefficient and can lead to |
| 109 | linear read overflows if a source string is not NUL-terminated. The |
| 110 | safe replacement is :c:func:`strscpy`. |
| 111 | |
Kees Cook | d8401f5 | 2020-03-04 23:03:47 -0800 | [diff] [blame] | 112 | %p format specifier |
| 113 | ------------------- |
| 114 | Traditionally, using "%p" in format strings would lead to regular address |
| 115 | exposure flaws in dmesg, proc, sysfs, etc. Instead of leaving these to |
| 116 | be exploitable, all "%p" uses in the kernel are being printed as a hashed |
| 117 | value, rendering them unusable for addressing. New uses of "%p" should not |
| 118 | be added to the kernel. For text addresses, using "%pS" is likely better, |
| 119 | as it produces the more useful symbol name instead. For nearly everything |
| 120 | else, just do not add "%p" at all. |
| 121 | |
| 122 | Paraphrasing Linus's current `guidance <https://lore.kernel.org/lkml/CA+55aFwQEd_d40g4mUCSsVRZzrFPUJt74vc6PPpb675hYNXcKw@mail.gmail.com/>`_: |
| 123 | |
| 124 | - If the hashed "%p" value is pointless, ask yourself whether the pointer |
| 125 | itself is important. Maybe it should be removed entirely? |
| 126 | - If you really think the true pointer value is important, why is some |
| 127 | system state or user privilege level considered "special"? If you think |
| 128 | you can justify it (in comments and commit log) well enough to stand |
| 129 | up to Linus's scrutiny, maybe you can use "%px", along with making sure |
| 130 | you have sensible permissions. |
| 131 | |
| 132 | And finally, know that a toggle for "%p" hashing will `not be accepted <https://lore.kernel.org/lkml/CA+55aFwieC1-nAs+NFq9RTwaR8ef9hWa4MjNBWL41F-8wM49eA@mail.gmail.com/>`_. |
| 133 | |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 134 | Variable Length Arrays (VLAs) |
| 135 | ----------------------------- |
| 136 | Using stack VLAs produces much worse machine code than statically |
| 137 | sized stack arrays. While these non-trivial `performance issues |
| 138 | <https://git.kernel.org/linus/02361bc77888>`_ are reason enough to |
| 139 | eliminate VLAs, they are also a security risk. Dynamic growth of a stack |
| 140 | array may exceed the remaining memory in the stack segment. This could |
| 141 | lead to a crash, possible overwriting sensitive contents at the end of the |
| 142 | stack (when built without `CONFIG_THREAD_INFO_IN_TASK=y`), or overwriting |
| 143 | memory adjacent to the stack (when built without `CONFIG_VMAP_STACK=y`) |
Gustavo A. R. Silva | a035d55 | 2019-06-06 19:46:17 -0500 | [diff] [blame] | 144 | |
| 145 | Implicit switch case fall-through |
| 146 | --------------------------------- |
Kees Cook | 76136e0 | 2020-03-04 11:03:24 -0800 | [diff] [blame^] | 147 | The C language allows switch cases to fall through to the next case |
| 148 | when a "break" statement is missing at the end of a case. This, however, |
| 149 | introduces ambiguity in the code, as it's not always clear if the missing |
| 150 | break is intentional or a bug. For example, it's not obvious just from |
| 151 | looking at the code if `STATE_ONE` is intentionally designed to fall |
| 152 | through into `STATE_TWO`:: |
| 153 | |
| 154 | switch (value) { |
| 155 | case STATE_ONE: |
| 156 | do_something(); |
| 157 | case STATE_TWO: |
| 158 | do_other(); |
| 159 | break; |
| 160 | default: |
| 161 | WARN("unknown state"); |
| 162 | } |
Joe Perches | b9918bd | 2019-10-05 09:46:43 -0700 | [diff] [blame] | 163 | |
| 164 | As there have been a long list of flaws `due to missing "break" statements |
Gustavo A. R. Silva | a035d55 | 2019-06-06 19:46:17 -0500 | [diff] [blame] | 165 | <https://cwe.mitre.org/data/definitions/484.html>`_, we no longer allow |
Kees Cook | 76136e0 | 2020-03-04 11:03:24 -0800 | [diff] [blame^] | 166 | implicit fall-through. In order to identify intentional fall-through |
| 167 | cases, we have adopted a pseudo-keyword macro "fallthrough" which |
| 168 | expands to gcc's extension `__attribute__((__fallthrough__)) |
| 169 | <https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html>`_. |
| 170 | (When the C17/C18 `[[fallthrough]]` syntax is more commonly supported by |
Joe Perches | b9918bd | 2019-10-05 09:46:43 -0700 | [diff] [blame] | 171 | C compilers, static analyzers, and IDEs, we can switch to using that syntax |
Kees Cook | 76136e0 | 2020-03-04 11:03:24 -0800 | [diff] [blame^] | 172 | for the macro pseudo-keyword.) |
Joe Perches | b9918bd | 2019-10-05 09:46:43 -0700 | [diff] [blame] | 173 | |
| 174 | All switch/case blocks must end in one of: |
| 175 | |
Kees Cook | 76136e0 | 2020-03-04 11:03:24 -0800 | [diff] [blame^] | 176 | * break; |
| 177 | * fallthrough; |
| 178 | * continue; |
| 179 | * goto <label>; |
| 180 | * return [expression]; |