On 17/12/2014 16:00, Yann Droneaud wrote: > Hi, > > Le mercredi 17 décembre 2014 à 08:54 +0200, Haggai Eran a écrit : >> On 16/12/2014 14:33, Yann Droneaud wrote: >>> Le jeudi 11 décembre 2014 à 17:04 +0200, Haggai Eran a écrit : >>>> static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, >>>> size_t len) >>>> { >>>> - return copy_to_user(udata->outbuf, src, len) ? -EFAULT : 0; >>>> + size_t copy_sz; >>>> + >>>> + copy_sz = min_t(size_t, len, udata->outlen); >>>> + return copy_to_user(udata->outbuf, src, copy_sz) ? -EFAULT : 0; >>>> } >>> >>> >>> This is not the place to do this: as I'm guessing the purpose of this >>> change from the patch in '[PATCH v3 07/17] IB/core: Add flags for on >>> demand paging support', you're trying to handle uverbs call from >>> a userspace program using a previous, shorter ABI. >> >> Yes, that was my intention. >> >>> >>> But that's hidding bug where userspace will get it wrong at passing the >>> correct buffer / size for all others uverb calls. >>> >>> That cannot work that way. >>> >>> In a previous patchset [1], I've suggested to add a check in >>> ib_copy_{from,to}_udata()[2][3] in order to check the input/output >>> buffer size to not read/write past userspace provided buffer >>> boundaries: in case of mismatch an error would be returned to >>> userspace. >>> >>> With the suggested change here, buffer overflow won't happen, >>> but the error is silently ignored, allowing uverb to return a >>> partial result, which is likely not expected by userspace as >>> it's a bit difficult to handle it gracefully. >>> >>> So this has to be removed, and a check on userspace response >>> buffer must be added to ib_uverbs_ex_query_device() instead. >> >> I agree that we shouldn't silently ignore bugs in userspace, but I'm not >> sure the alternative is maintainable. If we have in the future N new >> extensions to this verb, will we need to validate the user space given >> output buffer is one of the N possible sizes? >> > > Yes.
It would very easy for someone to forget one of the possible sizes, and thus blocking support for an older version of libibverbs. Such a bug would be hard to detect because it requires testing all previous versions of libibverbs. I think the problem you are trying to solve - userspace accidentally setting a smaller response size then required - will be detected immediately when one attempts to use that code. > > Additionnaly the size should be checked related to the flags set in the > "comp_mask": eg. requiring IB_USER_VERBS_EX_QUERY_DEVICE_ODP but not > providing the expected response buffer should be an error. In a query verb like EX_QUERY_DEVICE, I would expect the user-space code to request all bits in the comp_mask, since there's very little benefit from requesting a specific set (only a slightly shorter response for the system call). The kernel would ignore bits it doesn't know, and the user-space would ignore bits it doesn't know in the response. Regards, Haggai -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html