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