On Tue, Jul 28, 2020 at 12:34:45PM -0700, Kees Cook wrote: > On Mon, Jul 27, 2020 at 07:08:00PM -0400, Arvind Sankar wrote: > > The number of slots and slot areas can be unsigned int, since on 64-bit, > > the maximum amount of memory is 2^52, the minimum alignment is 2^21, so > > the slot number cannot be greater than 2^31. The slot areas are limited > > by MAX_SLOT_AREA, currently 100. Replace the type used for slot number, > > which is currently a mix of int and unsigned long, with unsigned int > > consistently. > > I think it's good to standardize the type, but let's go to unsigned long > then we don't have to think about this again in the future.
Ok, I can do that instead. > > > Drop unnecessary check that number of slots is not zero in > > store_slot_info, it's guaranteed to be at least 1 by the calculation. > > > > Drop unnecessary alignment of image_size to CONFIG_PHYSICAL_ALIGN in > > find_random_virt_addr, it cannot change the result: the largest valid > > slot is the largest n that satisfies > > I view all of these things as robustness checks. It doesn't hurt to do > these checks and it'll avoid crashing into problems if future > refactoring breaks assumptions. Well, at least the first one should really be unnecessary: the previous line sets it as 1 + x. When I see that it actually confuses me: I think I must be missing some edge case where it could be zero. The second one is also unnecessary, but I agree it might require a bit of analysis to see that it is. > > - slots = (KERNEL_IMAGE_SIZE - minimum - image_size) / > > - CONFIG_PHYSICAL_ALIGN + 1; > > + slots = 1 + (KERNEL_IMAGE_SIZE - minimum - image_size) / > > CONFIG_PHYSICAL_ALIGN; > > These are the same -- why change the code? > It's just reformatting now that we can have more than 80-column lines. I think it's clearer this way, more obvious that you're dividing by CONFIG_PHYSICAL_ALIGN and adding one, rather than "did he forget the parentheses here".
