On 08/14/2013 06:43 PM, Michal Židek wrote:
On 08/14/2013 04:51 PM, Simo Sorce wrote:
> On Tue, 2013-08-13 at 19:42 +0200, Michal Židek wrote:
>> Thanks for the review Simo.
>>
>> On 08/12/2013 11:07 PM, Simo Sorce wrote:
>
>>> What you need to check is somehing like:
>>> if (data->name > offsetof(struct sss_mc_pwd_data, strs) +
>>> data->strs_len) { return ENOENT; }
>>>
>>> ... except you should probably not trust strs_len entirely at this
>>> point
>>> if you are trying to catch malformed data and you should also check
>>> that
>>> data + strs_len is within the mmaped memory region.
>>>
>>
>> Ok. The new check tests if data + strs_len is in the data_table
>> (if it is somewhere else in the mmaped region it is already corrupted).
>
> Sure.
>
>>> Also at this point it may make sense to do a strlen(name) upfront and
>>> check that strs_len > name and return immediately if not.
>>>
>>
>> I added this one check too... I think it is not bad to have another
>> line of defense.
>
> Thanks.
>
>
>> Btw. I think we have off-by-one error in cases where we use pattern:
>> if (slot > MC_SIZE_TO_SLOTS(data_table_size) {
>> return something (ENOENT/NULL);
>> }
>>
>> If the slots are numbered from 0 and MC_SIZE_TO_SLOTS returns
>> number of slots needed to store some amount of data, there should
>> be '>=' no '>'. Please check my thinking. If I am correct
then the
>> second patch should fix it.
>
> Let's look at MC_SIZE_TO_SLOTS() definition:
>
> We always add (MC_SLOT_SIZE -1) to the requested size.
>
> This means if you ask 1 byte you get 1 slot, if you ask for MC_SLOT_SIZE
> + 1 you get 2 slots.
>
> Ie you get the right number of slots required for the size when you call
> that macro.
>
> So yeah good catch!
>
> However I think I'd like to see this fixed in a different way, by using
> a macro as we use this check elsewhere.
>
> Something like:
> #define MC_SLOT_WITHIN_BOUNDS(slot, size) \
> (slot <= (size / MC_SLOT_SIZE))
>
> and change the check to:
> if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { ...
>
> This would be more error proof if someone should add code in future to
> check bounds.
>
>> I also removed the triple check at Lukas's request in the second
>> patch, since it modifies the same parts already).
>
> I would prefer this to be done in a separate patch please.
>
> Simo.
>
Ok. I split the second patch into two. So now the second patch
is about the triple check and the third one about the off-by-one error.
The first patch is unchanged.
Thanks for the review
Michal
Sorry, the previous patchset included coverity issue that Lukas
mentioned. Sorry for the noise.
New patches attached.
Michal