On Thu, 2023-11-02 at 20:49 +0000, Al Viro wrote: > On Fri, Nov 03, 2023 at 06:24:09AM +1000, David Airlie wrote: > > On Fri, Nov 3, 2023 at 6:14 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> > > wrote: > > > > > > On Thu, Nov 02, 2023 at 08:21:35PM +0100, Philipp Stanner wrote: > > > > The functions (v)memdup_user() are utilized to copy userspace > > > > arrays. > > > > This is done without overflow checks. > > > > > > > > Use the new wrappers memdup_array_user() and > > > > vmemdup_array_user() to > > > > copy the arrays more safely. > > > > > > > @@ -644,7 +644,7 @@ int con_set_unimap(struct vc_data *vc, > > > > ushort ct, struct unipair __user *list) > > > > if (!ct) > > > > return 0; > > > > > > > - unilist = vmemdup_user(list, array_size(sizeof(*unilist), > > > > ct)); > > > > + unilist = vmemdup_array_user(list, ct, sizeof(*unilist)); > > > > if (IS_ERR(unilist)) > > > > return PTR_ERR(unilist); > > > > > > a 16bit value times sizeof(something). > > > > So since it's already using array_size here, moving it to a new > > helper > > for consistency just makes things clearer, and so you are fine with > > the patch? > > Sigh... OK, if you want it spelled out, there we go. I have no > objections > to the contents of patches; e.g. in case of ppp ioctl it saves the > reader > a grep in search of structure definitions, which is a good thing. > The one > and only suggestion I have for those patches is that such patches > might be > better off with explicit "in this case the overflow is avoided due to > <reasons>, but use of this helper makes it obviously safe" - or, in > case > of real bugs, "the overflow is, indeed, possible here", in which case > Fixes: ... and Cc: stable might be in order. > So if you agree the content is improving things a little bit then it seems the only critical thing is the commit message :) So let's get that fixed, shifting the focus from security to readability and general usefulness. Do you have a proposal for a good wording? Personally, I would have gone with something minimalistic like here in my other commit, where the irrelevance of the overflow-aspect was more obvious for me to see [1] I can also add a sentence clarifying that it's about improving readability or sth if you think that's better Kind regards, P. [1] https://lore.kernel.org/all/20231102192402.53721-2-pstanner@xxxxxxxxxx/
