On 26/02/18 19:32, J Freyensee wrote:

My replies also inlined.

On 2/26/18 4:09 AM, Igor Stoppa wrote:

[...]

But some of the code looks API'like to me, partly because of 
all the function header documentation, which thank you for that, but I 
wasn't sure where you drew your "API line" where the checks would be.

static and, even more, inlined static functions are not API, if found in
the .c file.

quoted

Is it assuming too much that the function will be used correctly, inside
the module it belongs to?

And even at API level, I'd tend to say that if there are chances that
the data received is corrupted, then it should be sanitized, but otherwise,
why adding overhead?

It's good secure coding practice to check your parameters, you are 
adding code to a security module after all ;-).

genalloc is not a security module :-P

it seems to be used in various places and for different purposes, also
depending on the architecture

For this reason I'm reluctant to add overhead.

If it's brand-new code entering the kernel, it's better to err on the 
side of having the extra checks and have a maintainer tell you to remove 
it than the other way around- especially since this code is part of the 
LSM solution.? What's worse- a tad bit of overhead catching a 
corner-case scenario that can be more easily fixed or something not 
caught that makes the kernel unstable?

ok, fair enough

[...]

quoted

If I really have to pick a place where to do the test, it's at API
level,

I agree, and if that is the case, I'm fine.

so I'll make sue that the API does sanitation

quoted

where the user of the API might fail to notice that the creation
of a pool failed and try to get memory from a non-existing pool.
That is the only scenario I can think of, where bogus data would be
received.

quoted

quoted

   	return chunk->end_addr - chunk->start_addr + 1;
   }
   
-static int set_bits_ll(unsigned long *addr, unsigned long mask_to_set)
+
+/**
+ * set_bits_ll() - based on value and mask, sets bits at address
+ * @addr: where to write
+ * @mask: filter to apply for the bits to alter
+ * @value: actual configuration of bits to store
+ *
+ * Return:
+ * * 0		- success
+ * * -EBUSY	- otherwise
+ */
+static int set_bits_ll(unsigned long *addr,
+		       unsigned long mask, unsigned long value)
   {
-	unsigned long val, nval;
+	unsigned long nval;
+	unsigned long present;
+	unsigned long target;
   
   	nval = *addr;

Same issue here with addr.

Again, I am more leaning toward believing that the user of the API might
forget to check for errors,

Same in agreement, so if that is the case, I'm ok.? It was a little hard 
to tell what is exactly your API is.? I'm used to reviewing kernel code 
where important API-like functions were heavily documented, and inner 
routines were not...so seeing the function documentation (which is a 
good thing :-)) made me think this was some sort of new API code I was 
looking at.

it's static, therefore no API

quoted

and pass a NULL pointer as pool, than to
believe something like this would happen.

This is an address obtained from data managed automatically by the library.

Can you please explain why you think it would be NULL?

Why would it be NULL?? I don't know, I'm not intimately familiar with 
the code; but I default to implementing code defensively.? But I'll turn 
the question around on you- why would it NOT be NULL?? Are you sure this 
will never be NULL?? Are you going to trust the library that it always 
provides a good address?? You should add to your function header 
documentation why addr will NOT be NULL.

ok, I can add the explanation
which is: the corresponding memory is allocated when a pool is created.
should the allocation fail, the pool creation will fail consequently

The only cases which can cause this to be NULL within a pool are:
* accidental corruption
* attacker tampering with kernel memory

However they are both quite unlikely:
* accidental corruption should not happen so easily and, in case it
happens, it's likely to plow also some surrounding memory.
* this is just metadata, supposed to be useful mostly before the pool is
write-protected. If an attacker is capable of altering arbitrary kernel
data memory, there are far better targets.

[...]

quoted

quoted

quoted

+	/*
+	 * Prepare for writing the initial part of the allocation, from
+	 * starting entry, to the end of the UL bitmap element which
+	 * contains it. It might be larger than the actual allocation.
+	 */
+	start_bit = ENTRIES_TO_BITS(start_entry);
+	end_bit = ENTRIES_TO_BITS(start_entry + nentries);
+	nbits = ENTRIES_TO_BITS(nentries);

these statements won't make any sense if start_entry and nentries are
negative values, which is possible based on the function definition
alter_bitmap_ll().? Am I missing something that it's ok for these
parameters to be negative?

This patch is extending the handling of the bitmap, it's not trying to
rewrite genalloc, thus it tries to not alter parts which are unrelated.
Like the type of parameters passed.

What you are suggesting is a further cleanup of genalloc.
I'm not against it, but it's unrelated to this patchset.

OK, very reasonable.? Then I would think this would be a case to add a 
check for negative values in the function parameters start_entry and 
nentries as it's possible (though maybe not realistic) to have negative 
values supplied, especially if there is currently no active maintainer 
for genalloc().? Since you are fitting new code to genalloc's behavior 
and this is a security module, I'll err on the side of checking the 
parameters for bad values, or document in your function header comments 
why it is expected for these parameters to never have negative values.

I'll figure out which alternative I dislike the least :-)

Probably just fix the data types in a separate patch.
This patch for genalloc has generated various comments which are
actually more about the original implementation than what I'm adding.


--
igor
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html