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 | |
Kees Cook | 7af5167 | 2020-03-14 15:29:50 -0700 | [diff] [blame] | 32 | BUG() and BUG_ON() |
| 33 | ------------------ |
| 34 | Use WARN() and WARN_ON() instead, and handle the "impossible" |
| 35 | error condition as gracefully as possible. While the BUG()-family |
| 36 | of APIs were originally designed to act as an "impossible situation" |
| 37 | assert and to kill a kernel thread "safely", they turn out to just be |
| 38 | too risky. (e.g. "In what order do locks need to be released? Have |
| 39 | various states been restored?") Very commonly, using BUG() will |
| 40 | destabilize a system or entirely break it, which makes it impossible |
| 41 | to debug or even get viable crash reports. Linus has `very strong |
| 42 | <https://lore.kernel.org/lkml/CA+55aFy6jNLsywVYdGp83AMrXBo_P-pkjkphPGrO=82SPKCpLQ@mail.gmail.com/>`_ |
| 43 | feelings `about this |
| 44 | <https://lore.kernel.org/lkml/CAHk-=whDHsbK3HTOpTF=ue_o04onRwTEaK_ZoJp_fjbqq4+=Jw@mail.gmail.com/>`_. |
| 45 | |
| 46 | Note that the WARN()-family should only be used for "expected to |
| 47 | be unreachable" situations. If you want to warn about "reachable |
| 48 | but undesirable" situations, please use the pr_warn()-family of |
| 49 | functions. System owners may have set the *panic_on_warn* sysctl, |
| 50 | to make sure their systems do not continue running in the face of |
| 51 | "unreachable" conditions. (For example, see commits like `this one |
| 52 | <https://git.kernel.org/linus/d4689846881d160a4d12a514e991a740bcb5d65a>`_.) |
| 53 | |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 54 | open-coded arithmetic in allocator arguments |
| 55 | -------------------------------------------- |
| 56 | Dynamic size calculations (especially multiplication) should not be |
| 57 | performed in memory allocator (or similar) function arguments due to the |
| 58 | risk of them overflowing. This could lead to values wrapping around and a |
| 59 | smaller allocation being made than the caller was expecting. Using those |
| 60 | allocations could lead to linear overflows of heap memory and other |
| 61 | misbehaviors. (One exception to this is literal values where the compiler |
| 62 | can warn if they might overflow. Though using literals for arguments as |
| 63 | suggested below is also harmless.) |
| 64 | |
| 65 | For example, do not use ``count * size`` as an argument, as in:: |
| 66 | |
| 67 | foo = kmalloc(count * size, GFP_KERNEL); |
| 68 | |
| 69 | Instead, the 2-factor form of the allocator should be used:: |
| 70 | |
| 71 | foo = kmalloc_array(count, size, GFP_KERNEL); |
| 72 | |
| 73 | If no 2-factor form is available, the saturate-on-overflow helpers should |
| 74 | be used:: |
| 75 | |
| 76 | bar = vmalloc(array_size(count, size)); |
| 77 | |
| 78 | Another common case to avoid is calculating the size of a structure with |
| 79 | a trailing array of others structures, as in:: |
| 80 | |
| 81 | header = kzalloc(sizeof(*header) + count * sizeof(*header->item), |
| 82 | GFP_KERNEL); |
| 83 | |
| 84 | Instead, use the helper:: |
| 85 | |
| 86 | header = kzalloc(struct_size(header, item, count), GFP_KERNEL); |
| 87 | |
Gustavo A. R. Silva | 68e4cd1 | 2020-06-08 16:37:11 -0500 | [diff] [blame] | 88 | .. note:: If you are using struct_size() on a structure containing a zero-length |
| 89 | or a one-element array as a trailing array member, please refactor such |
| 90 | array usage and switch to a `flexible array member |
| 91 | <#zero-length-and-one-element-arrays>`_ instead. |
| 92 | |
Jonathan Corbet | 7929b98 | 2020-03-10 11:27:22 -0600 | [diff] [blame] | 93 | See array_size(), array3_size(), and struct_size(), |
| 94 | for more details as well as the related check_add_overflow() and |
| 95 | check_mul_overflow() family of functions. |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 96 | |
| 97 | simple_strtol(), simple_strtoll(), simple_strtoul(), simple_strtoull() |
| 98 | ---------------------------------------------------------------------- |
Jonathan Corbet | 7929b98 | 2020-03-10 11:27:22 -0600 | [diff] [blame] | 99 | The simple_strtol(), simple_strtoll(), |
| 100 | simple_strtoul(), and simple_strtoull() functions |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 101 | explicitly ignore overflows, which may lead to unexpected results |
Jonathan Corbet | 7929b98 | 2020-03-10 11:27:22 -0600 | [diff] [blame] | 102 | in callers. The respective kstrtol(), kstrtoll(), |
| 103 | kstrtoul(), and kstrtoull() functions tend to be the |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 104 | correct replacements, though note that those require the string to be |
| 105 | NUL or newline terminated. |
| 106 | |
| 107 | strcpy() |
| 108 | -------- |
Kees Cook | 27def95 | 2020-10-15 16:17:31 -0700 | [diff] [blame] | 109 | strcpy() performs no bounds checking on the destination buffer. This |
| 110 | could result in linear overflows beyond the end of the buffer, leading to |
| 111 | all kinds of misbehaviors. While `CONFIG_FORTIFY_SOURCE=y` and various |
| 112 | compiler flags help reduce the risk of using this function, there is |
| 113 | no good reason to add new uses of this function. The safe replacement |
| 114 | is strscpy(), though care must be given to any cases where the return |
| 115 | value of strcpy() was used, since strscpy() does not return a pointer to |
| 116 | the destination, but rather a count of non-NUL bytes copied (or negative |
| 117 | errno when it truncates). |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 118 | |
| 119 | strncpy() on NUL-terminated strings |
| 120 | ----------------------------------- |
Kees Cook | 27def95 | 2020-10-15 16:17:31 -0700 | [diff] [blame] | 121 | Use of strncpy() does not guarantee that the destination buffer will |
| 122 | be NUL terminated. This can lead to various linear read overflows and |
| 123 | other misbehavior due to the missing termination. It also NUL-pads |
| 124 | the destination buffer if the source contents are shorter than the |
| 125 | destination buffer size, which may be a needless performance penalty |
| 126 | for callers using only NUL-terminated strings. The safe replacement is |
| 127 | strscpy(), though care must be given to any cases where the return value |
| 128 | of strncpy() was used, since strscpy() does not return a pointer to the |
| 129 | destination, but rather a count of non-NUL bytes copied (or negative |
| 130 | errno when it truncates). Any cases still needing NUL-padding should |
| 131 | instead use strscpy_pad(). |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 132 | |
Kees Cook | 053f8fc | 2020-08-17 16:32:07 -0700 | [diff] [blame] | 133 | If a caller is using non-NUL-terminated strings, strncpy() can |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 134 | still be used, but destinations should be marked with the `__nonstring |
| 135 | <https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_ |
| 136 | attribute to avoid future compiler warnings. |
| 137 | |
| 138 | strlcpy() |
| 139 | --------- |
Kees Cook | 27def95 | 2020-10-15 16:17:31 -0700 | [diff] [blame] | 140 | strlcpy() reads the entire source buffer first (since the return value |
| 141 | is meant to match that of strlen()). This read may exceed the destination |
| 142 | size limit. This is both inefficient and can lead to linear read overflows |
| 143 | if a source string is not NUL-terminated. The safe replacement is strscpy(), |
| 144 | though care must be given to any cases where the return value of strlcpy() |
| 145 | is used, since strscpy() will return negative errno values when it truncates. |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 146 | |
Kees Cook | d8401f5 | 2020-03-04 23:03:47 -0800 | [diff] [blame] | 147 | %p format specifier |
| 148 | ------------------- |
| 149 | Traditionally, using "%p" in format strings would lead to regular address |
| 150 | exposure flaws in dmesg, proc, sysfs, etc. Instead of leaving these to |
| 151 | be exploitable, all "%p" uses in the kernel are being printed as a hashed |
| 152 | value, rendering them unusable for addressing. New uses of "%p" should not |
| 153 | be added to the kernel. For text addresses, using "%pS" is likely better, |
| 154 | as it produces the more useful symbol name instead. For nearly everything |
| 155 | else, just do not add "%p" at all. |
| 156 | |
| 157 | Paraphrasing Linus's current `guidance <https://lore.kernel.org/lkml/CA+55aFwQEd_d40g4mUCSsVRZzrFPUJt74vc6PPpb675hYNXcKw@mail.gmail.com/>`_: |
| 158 | |
| 159 | - If the hashed "%p" value is pointless, ask yourself whether the pointer |
| 160 | itself is important. Maybe it should be removed entirely? |
| 161 | - If you really think the true pointer value is important, why is some |
| 162 | system state or user privilege level considered "special"? If you think |
| 163 | you can justify it (in comments and commit log) well enough to stand |
| 164 | up to Linus's scrutiny, maybe you can use "%px", along with making sure |
| 165 | you have sensible permissions. |
| 166 | |
Kees Cook | 6ab0493 | 2021-07-23 13:05:26 -0700 | [diff] [blame] | 167 | If you are debugging something where "%p" hashing is causing problems, |
| 168 | you can temporarily boot with the debug flag "`no_hash_pointers |
| 169 | <https://git.kernel.org/linus/5ead723a20e0447bc7db33dc3070b420e5f80aa6>`_". |
Kees Cook | d8401f5 | 2020-03-04 23:03:47 -0800 | [diff] [blame] | 170 | |
Kees Cook | 84253c8 | 2018-10-17 16:45:32 -0700 | [diff] [blame] | 171 | Variable Length Arrays (VLAs) |
| 172 | ----------------------------- |
| 173 | Using stack VLAs produces much worse machine code than statically |
| 174 | sized stack arrays. While these non-trivial `performance issues |
| 175 | <https://git.kernel.org/linus/02361bc77888>`_ are reason enough to |
| 176 | eliminate VLAs, they are also a security risk. Dynamic growth of a stack |
| 177 | array may exceed the remaining memory in the stack segment. This could |
| 178 | lead to a crash, possible overwriting sensitive contents at the end of the |
| 179 | stack (when built without `CONFIG_THREAD_INFO_IN_TASK=y`), or overwriting |
| 180 | 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] | 181 | |
| 182 | Implicit switch case fall-through |
| 183 | --------------------------------- |
Kees Cook | 76136e0 | 2020-03-04 11:03:24 -0800 | [diff] [blame] | 184 | The C language allows switch cases to fall through to the next case |
| 185 | when a "break" statement is missing at the end of a case. This, however, |
| 186 | introduces ambiguity in the code, as it's not always clear if the missing |
| 187 | break is intentional or a bug. For example, it's not obvious just from |
| 188 | looking at the code if `STATE_ONE` is intentionally designed to fall |
| 189 | through into `STATE_TWO`:: |
| 190 | |
| 191 | switch (value) { |
| 192 | case STATE_ONE: |
| 193 | do_something(); |
| 194 | case STATE_TWO: |
| 195 | do_other(); |
| 196 | break; |
| 197 | default: |
| 198 | WARN("unknown state"); |
| 199 | } |
Joe Perches | b9918bd | 2019-10-05 09:46:43 -0700 | [diff] [blame] | 200 | |
| 201 | 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] | 202 | <https://cwe.mitre.org/data/definitions/484.html>`_, we no longer allow |
Kees Cook | 76136e0 | 2020-03-04 11:03:24 -0800 | [diff] [blame] | 203 | implicit fall-through. In order to identify intentional fall-through |
| 204 | cases, we have adopted a pseudo-keyword macro "fallthrough" which |
| 205 | expands to gcc's extension `__attribute__((__fallthrough__)) |
| 206 | <https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html>`_. |
| 207 | (When the C17/C18 `[[fallthrough]]` syntax is more commonly supported by |
Joe Perches | b9918bd | 2019-10-05 09:46:43 -0700 | [diff] [blame] | 208 | 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] | 209 | for the macro pseudo-keyword.) |
Joe Perches | b9918bd | 2019-10-05 09:46:43 -0700 | [diff] [blame] | 210 | |
| 211 | All switch/case blocks must end in one of: |
| 212 | |
Kees Cook | 76136e0 | 2020-03-04 11:03:24 -0800 | [diff] [blame] | 213 | * break; |
| 214 | * fallthrough; |
| 215 | * continue; |
| 216 | * goto <label>; |
| 217 | * return [expression]; |
Gustavo A. R. Silva | 68e4cd1 | 2020-06-08 16:37:11 -0500 | [diff] [blame] | 218 | |
| 219 | Zero-length and one-element arrays |
| 220 | ---------------------------------- |
| 221 | There is a regular need in the kernel to provide a way to declare having |
| 222 | a dynamically sized set of trailing elements in a structure. Kernel code |
| 223 | should always use `"flexible array members" <https://en.wikipedia.org/wiki/Flexible_array_member>`_ |
| 224 | for these cases. The older style of one-element or zero-length arrays should |
| 225 | no longer be used. |
| 226 | |
| 227 | In older C code, dynamically sized trailing elements were done by specifying |
| 228 | a one-element array at the end of a structure:: |
| 229 | |
| 230 | struct something { |
| 231 | size_t count; |
| 232 | struct foo items[1]; |
| 233 | }; |
| 234 | |
| 235 | This led to fragile size calculations via sizeof() (which would need to |
| 236 | remove the size of the single trailing element to get a correct size of |
| 237 | the "header"). A `GNU C extension <https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html>`_ |
| 238 | was introduced to allow for zero-length arrays, to avoid these kinds of |
| 239 | size problems:: |
| 240 | |
| 241 | struct something { |
| 242 | size_t count; |
| 243 | struct foo items[0]; |
| 244 | }; |
| 245 | |
| 246 | But this led to other problems, and didn't solve some problems shared by |
| 247 | both styles, like not being able to detect when such an array is accidentally |
| 248 | being used _not_ at the end of a structure (which could happen directly, or |
| 249 | when such a struct was in unions, structs of structs, etc). |
| 250 | |
| 251 | C99 introduced "flexible array members", which lacks a numeric size for |
| 252 | the array declaration entirely:: |
| 253 | |
| 254 | struct something { |
| 255 | size_t count; |
| 256 | struct foo items[]; |
| 257 | }; |
| 258 | |
| 259 | This is the way the kernel expects dynamically sized trailing elements |
| 260 | to be declared. It allows the compiler to generate errors when the |
| 261 | flexible array does not occur last in the structure, which helps to prevent |
| 262 | some kind of `undefined behavior |
| 263 | <https://git.kernel.org/linus/76497732932f15e7323dc805e8ea8dc11bb587cf>`_ |
| 264 | bugs from being inadvertently introduced to the codebase. It also allows |
| 265 | the compiler to correctly analyze array sizes (via sizeof(), |
| 266 | `CONFIG_FORTIFY_SOURCE`, and `CONFIG_UBSAN_BOUNDS`). For instance, |
| 267 | there is no mechanism that warns us that the following application of the |
| 268 | sizeof() operator to a zero-length array always results in zero:: |
| 269 | |
| 270 | struct something { |
| 271 | size_t count; |
| 272 | struct foo items[0]; |
| 273 | }; |
| 274 | |
| 275 | struct something *instance; |
| 276 | |
| 277 | instance = kmalloc(struct_size(instance, items, count), GFP_KERNEL); |
| 278 | instance->count = count; |
| 279 | |
| 280 | size = sizeof(instance->items) * instance->count; |
| 281 | memcpy(instance->items, source, size); |
| 282 | |
| 283 | At the last line of code above, ``size`` turns out to be ``zero``, when one might |
| 284 | have thought it represents the total size in bytes of the dynamic memory recently |
| 285 | allocated for the trailing array ``items``. Here are a couple examples of this |
| 286 | issue: `link 1 |
| 287 | <https://git.kernel.org/linus/f2cd32a443da694ac4e28fbf4ac6f9d5cc63a539>`_, |
| 288 | `link 2 |
| 289 | <https://git.kernel.org/linus/ab91c2a89f86be2898cee208d492816ec238b2cf>`_. |
| 290 | Instead, `flexible array members have incomplete type, and so the sizeof() |
| 291 | operator may not be applied <https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html>`_, |
| 292 | so any misuse of such operators will be immediately noticed at build time. |
| 293 | |
| 294 | With respect to one-element arrays, one has to be acutely aware that `such arrays |
| 295 | occupy at least as much space as a single object of the type |
| 296 | <https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html>`_, |
| 297 | hence they contribute to the size of the enclosing structure. This is prone |
| 298 | to error every time people want to calculate the total size of dynamic memory |
| 299 | to allocate for a structure containing an array of this kind as a member:: |
| 300 | |
| 301 | struct something { |
| 302 | size_t count; |
| 303 | struct foo items[1]; |
| 304 | }; |
| 305 | |
| 306 | struct something *instance; |
| 307 | |
| 308 | instance = kmalloc(struct_size(instance, items, count - 1), GFP_KERNEL); |
| 309 | instance->count = count; |
| 310 | |
| 311 | size = sizeof(instance->items) * instance->count; |
| 312 | memcpy(instance->items, source, size); |
| 313 | |
| 314 | In the example above, we had to remember to calculate ``count - 1`` when using |
| 315 | the struct_size() helper, otherwise we would have --unintentionally-- allocated |
| 316 | memory for one too many ``items`` objects. The cleanest and least error-prone way |
Gustavo A. R. Silva | 17dca05 | 2020-08-31 20:09:49 -0500 | [diff] [blame] | 317 | to implement this is through the use of a `flexible array member`, together with |
| 318 | struct_size() and flex_array_size() helpers:: |
Gustavo A. R. Silva | 68e4cd1 | 2020-06-08 16:37:11 -0500 | [diff] [blame] | 319 | |
| 320 | struct something { |
| 321 | size_t count; |
| 322 | struct foo items[]; |
| 323 | }; |
| 324 | |
| 325 | struct something *instance; |
| 326 | |
| 327 | instance = kmalloc(struct_size(instance, items, count), GFP_KERNEL); |
| 328 | instance->count = count; |
| 329 | |
Gustavo A. R. Silva | 17dca05 | 2020-08-31 20:09:49 -0500 | [diff] [blame] | 330 | memcpy(instance->items, source, flex_array_size(instance, items, instance->count)); |