Messages in this thread Patch in this message | | | Subject | Re: [PATCH] drivers/tty/vt: copy userspace arrays safely | From | Philipp Stanner <> | Date | Thu, 02 Nov 2023 23:07:51 +0100 |
| |
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@zeniv.linux.org.uk> > > 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@redhat.com/
| |