diff mbox series

drivers/tty/vt: copy userspace arrays safely

Message ID 20231102192134.53301-2-pstanner@redhat.com
State New
Headers
Series drivers/tty/vt: copy userspace arrays safely |

Commit Message

Philipp Stanner Nov. 2, 2023, 7:21 p.m. UTC 
  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 <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/tty/vt/consolemap.c | 2 +-
 drivers/tty/vt/keyboard.c   | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Al Viro Nov. 2, 2023, 8:14 p.m. UTC | #1 
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));
David Airlie Nov. 2, 2023, 8:24 p.m. UTC | #2 
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?

Otherwise I'd think you are been a snarky asshole to a coworker.

Dave.
Al Viro Nov. 2, 2023, 8:49 p.m. UTC | #3 
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.
Philipp Stanner Nov. 2, 2023, 10:07 p.m. UTC | #4 
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/
diff mbox series

Patch

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);
 		}