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.
Suggested-by: Dave Airlie <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/tty/vt/consolemap.c | 2 +-
drivers/tty/vt/keyboard.c | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index f02d21e2a96e..313cef3322eb 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -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);
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 1fe6107b539b..802ceb0a5e4c 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -1773,8 +1773,8 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
if (ct) {
- dia = memdup_user(a->kbdiacr,
- sizeof(struct kbdiacr) * ct);
+ dia = memdup_array_user(a->kbdiacr,
+ ct, sizeof(struct kbdiacr));
if (IS_ERR(dia))
return PTR_ERR(dia);
@@ -1811,8 +1811,8 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
return -EINVAL;
if (ct) {
- buf = memdup_user(a->kbdiacruc,
- ct * sizeof(struct kbdiacruc));
+ buf = memdup_array_user(a->kbdiacruc,
+ ct, sizeof(struct kbdiacruc));
if (IS_ERR(buf))
return PTR_ERR(buf);
}
--
2.41.0
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).
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 1fe6107b539b..802ceb0a5e4c 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -1773,8 +1773,8 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
... and here we have
if (ct >= MAX_DIACR)
return -EINVAL;
directly upstream, so it's even better - a value below 256 times sizeof(something)
> if (ct) {
>
> - dia = memdup_user(a->kbdiacr,
> - sizeof(struct kbdiacr) * ct);
> + dia = memdup_array_user(a->kbdiacr,
> + ct, sizeof(struct kbdiacr));
> if (IS_ERR(dia))
> return PTR_ERR(dia);
>
> @@ -1811,8 +1811,8 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
> return -EINVAL;
Ditto.
> if (ct) {
> - buf = memdup_user(a->kbdiacruc,
> - ct * sizeof(struct kbdiacruc));
> + buf = memdup_array_user(a->kbdiacruc,
> + ct, sizeof(struct kbdiacruc));
On Fri, Nov 3, 2023 at 6:14 AM Al Viro <[email protected]> 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?
Otherwise I'd think you are been a snarky asshole to a coworker.
Dave.
On Fri, Nov 03, 2023 at 06:24:09AM +1000, David Airlie wrote:
> On Fri, Nov 3, 2023 at 6:14 AM Al Viro <[email protected]> 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.
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 <[email protected]>
> > 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/[email protected]/