Hello!
On May 14, 2015, at 5:52 AM, Gujulan Elango, Hari Prasath (H.) wrote:
> On Thu, May 14, 2015 at 12:47:19PM +0300, Dan Carpenter wrote:
>> On Thu, May 14, 2015 at 09:22:01AM +0000, Gujulan Elango, Hari Prasath (H.) wrote:
>>> The return value of copy_to_user() isn't checked for failure.Hence
>>> return -EFAULT if it fails.
>>>
>>> Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@xxxxxxxxxxx>
>>> ---
>>> drivers/staging/lustre/lustre/lov/lov_pack.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/staging/lustre/lustre/lov/lov_pack.c b/drivers/staging/lustre/lustre/lov/lov_pack.c
>>> index 5356d53..aa3d6de 100644
>>> --- a/drivers/staging/lustre/lustre/lov/lov_pack.c
>>> +++ b/drivers/staging/lustre/lustre/lov/lov_pack.c
>>> @@ -448,9 +448,10 @@ int lov_getstripe(struct obd_export *exp, struct lov_stripe_md *lsm,
>>> (lum.lmm_stripe_count < lsm->lsm_stripe_count)) {
>>> /* Return right size of stripe to user */
>>> lum.lmm_stripe_count = lsm->lsm_stripe_count;
>>> - rc = copy_to_user(lump, &lum, lum_size);
>>> - rc = -EOVERFLOW;
>>> - goto out_set;
>>> + if (copy_to_user(lump, &lum, lum_size)) {
>>> + rc = -EFAULT;
>>> + goto out_set;
>>> + }
This is bad if only because now if copy to user succeeds we are NOT exiting right here, which
would have lead to buffer overflow in userspace, except we check for this later on
down this path once the striping info is actually refreshed.
Still, it's wrong.
>> I'm not sure this is right, and I don't think we should take it without
>> some lustre people signing off.
>>
>> The original code looks deliberate, like the error happened earlier and
>> if the copy_to_user() succeed that's fine because it gives them a hint
>> what went wrong, but we want to return an error -EOVERFLOW regardless of
>> if the copy_to_user() works or not.
>>
> I was little skeptical when sending this patch.I agree with you and we
> shall wait for comments from lustre developers.May be if it was
> purposeful,they could have left out a comment.
Well, there IS a comment. - "Return right size of stripe to user"
Basically what happens here is the userspace told us "hey, what's the striping on this file?
we are ready to accept up to XX size", so here we are on a path where the striping data is bigger than
the buffer supplied. So we fill the header with information about how big the striping is and
return.
Now, lum_size at this point is
lum_size = sizeof(struct lov_user_md_v1);
And we even know that we were able to read it before:
if (copy_from_user(&lum, lump, lum_size))
GOTO(out, rc = -EFAULT);
So I imagine the only case where this could fail is if we got a pointer to some read-only location
(or there's somebody playing with mappings behind our back?) so it should be totally fine to
do it the other way around:
rc = -EOVERFLOW;
if (copy_to_user(lump, &lum, lum_size))
rc = -EFAULT;
goto out_set;
Bye,
Oleg
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
