On Wed, Oct 17, 2018 at 5:50 AM, Jonathan Corbet <corbet@xxxxxxx> wrote: > On Tue, 16 Oct 2018 19:17:08 -0700 > Kees Cook <keescook@xxxxxxxxxxxx> wrote: > >> As discussed in the "API replacement/deprecation" thread[1], this >> makes an effort to document what things shouldn't get (re)added to the >> kernel, by introducing Documentation/process/deprecated.rst. It also >> adds the overflow kerndoc to ReST output, and tweaks the struct_size() >> documentation to parse correctly. > > Seems like a good idea overall...a couple of quick comments > >> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html >> >> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> >> --- >> Documentation/driver-api/basics.rst | 3 + >> Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++ >> Documentation/process/index.rst | 1 + > > I wonder if "process" is the right place, or core-api? I guess we have a > lot of similar stuff in process now. Totally up to you. It seemed better suited for "process" (here's things NOT to do) than "core-api" (here's things to use). > > [...] > >> +open-coded arithmetic in allocator arguments >> +-------------------------------------------- >> +Dynamic size calculations (especially multiplication) >> +should not be performed in memory allocator (or similar) >> +function arguments. >> + >> +For example, do not use ``rows * cols`` as an argument, as in: >> +``kmalloc(rows * cols, GFP_KERNEL)``. >> +Instead, the 2-factor form of the allocator should be used: >> +``kmalloc_array(rows, cols, GFP_KERNEL)``. >> +If no 2-factor form is available, the saturate-on-overflow helpers should >> +be used: >> +``vmalloc(array_size(rows, cols))``. >> + >> +See :c:func:`array_size`, :c:func:`array3_size`, and :c:func:`struct_size`, >> +for more details as well as the related :c:func:`check_add_overflow` and >> +:c:func:`check_mul_overflow` family of functions. > > I think this should say *why* developers are being told not to do it. Does > this advice hold even in cases where the dimensions are known, under the > kernel's control, and guaranteed not to overflow even on Alan's port to > eight-bit CPUs? I will attempt to explain this better. When all factors are constants, the compiler will warn if there is an overflow. If, however, anything is a dynamic size, we run the risk of overflow. It's not true in all cases (e.g. u8 var * sizeof()) but it's more robust to globally avoid it. (What happens when that u8 becomes a u64 at some future time?) > To me it's also a bit confusing to present the arguments to kmalloc_array() > as "rows" and "cols" when they are really "n" and "size". That's true, though I used rows * cols as an example because people aren't always thinking about their multiplications as having n-many sizes. e.g. "I just want a full screen of pixels." Let me see if I can adjust it... Thanks for the feedback! -Kees -- Kees Cook Pixel Security
