From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yann Droneaud Subject: Re: [PATCH v3 06/17] IB/core: Add support for extended query device caps Date: Wed, 17 Dec 2014 15:00:11 +0100 Message-ID: <1418824811.3334.3.camel@dworkin> References: <1418310266-9584-1-git-send-email-haggaie@mellanox.com> <1418310266-9584-7-git-send-email-haggaie@mellanox.com> <1418733236.2779.26.camel@opteya.com> <549128B4.5010301@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <549128B4.5010301-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Haggai Eran Cc: Roland Dreier , linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Liran Liss , Or Gerlitz , Sagi Grimberg , Majd Dibbiny , Jerome Glisse , Eli Cohen List-Id: linux-rdma@vger.kernel.org Hi, Le mercredi 17 d=C3=A9cembre 2014 =C3=A0 08:54 +0200, Haggai Eran a =C3= =A9crit : > On 16/12/2014 14:33, Yann Droneaud wrote: > > Le jeudi 11 d=C3=A9cembre 2014 =C3=A0 17:04 +0200, Haggai Eran a =C3= =A9crit : > >> 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 =3D min_t(size_t, len, udata->outlen); > >> + return copy_to_user(udata->outbuf, src, copy_sz) ? -EFAULT : 0; > >> } > >=20 > >=20 > > This is not the place to do this: as I'm guessing the purpose of th= is=20 > > change from the patch in '[PATCH v3 07/17] IB/core: Add flags for o= n=20 > > demand paging support', you're trying to handle uverbs call from=20 > > a userspace program using a previous, shorter ABI. >=20 > Yes, that was my intention. >=20 > >=20 > > But that's hidding bug where userspace will get it wrong at passing= the=20 > > correct buffer / size for all others uverb calls. > >=20 > > That cannot work that way. > >=20 > > In a previous patchset [1], I've suggested to add a check in=20 > > 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. > >=20 > > 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. > >=20 > > So this has to be removed, and a check on userspace response > > buffer must be added to ib_uverbs_ex_query_device() instead. >=20 > 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 give= n > output buffer is one of the N possible sizes? >=20 Yes. 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. Regards. --=20 Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" i= n the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html