From owner-freebsd-bugs@freebsd.org Tue May 30 19:47:27 2017 Return-Path: Delivered-To: freebsd-bugs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id BE93ABDA7B8 for ; Tue, 30 May 2017 19:47:27 +0000 (UTC) (envelope-from bugzilla-noreply@freebsd.org) Received: from kenobi.freebsd.org (kenobi.freebsd.org [IPv6:2001:1900:2254:206a::16:76]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id A2BD276A1B for ; Tue, 30 May 2017 19:47:27 +0000 (UTC) (envelope-from bugzilla-noreply@freebsd.org) Received: from bugs.freebsd.org ([127.0.1.118]) by kenobi.freebsd.org (8.15.2/8.15.2) with ESMTP id v4UJlRGf092442 for ; Tue, 30 May 2017 19:47:27 GMT (envelope-from bugzilla-noreply@freebsd.org) From: bugzilla-noreply@freebsd.org To: freebsd-bugs@FreeBSD.org Subject: [Bug 219525] Multiple bugs in mpr ioctl handler Date: Tue, 30 May 2017 19:47:27 +0000 X-Bugzilla-Reason: AssignedTo X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: Base System X-Bugzilla-Component: kern X-Bugzilla-Version: CURRENT X-Bugzilla-Keywords: X-Bugzilla-Severity: Affects Only Me X-Bugzilla-Who: slm@freebsd.org X-Bugzilla-Status: New X-Bugzilla-Resolution: X-Bugzilla-Priority: --- X-Bugzilla-Assigned-To: freebsd-bugs@FreeBSD.org X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: https://bugs.freebsd.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 May 2017 19:47:27 -0000 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=3D219525 --- Comment #1 from Stephen McConnell --- Thanks for the report. I know there are a few issues there, especially with= the driver not checking for bad user supplied values. I agree with most of your comments, but I have a question. In this section: ..... mpr_lock(sc); size =3D data->Size; if ((size >=3D sizeof(sc->recorded_events)) && (status =3D=3D 0)) { mpr_unlock(sc); if (copyout((void *)sc->recorded_events, PTRIN(data->PtrEvents), size) !=3D 0) status =3D EFAULT; mpr_lock(sc); } else { /* * data->Size value is not large enough to copy event data. */ status =3D EFAULT; } /* * Change size value to match the number of bytes that were copied. */ if (status =3D=3D 0) data->Size =3D sizeof(sc->recorded_events); mpr_unlock(sc); ..... You say: "It checks if the user supplied size is greater than the the size of the struct, and if so, copies using the size which it just determined was too large. It should `copyout` using `sizeof(sc->recorded_events)`." But this is just checking if the user supplied size is enough to hold sizeof(sc->recored_events). It's not checking if it was too large, it's checking if it's large enough. If that user supplied size is >=3D to the da= ta being copied, it should be OK. Do you agree? I'm not sure I like that data->Size is being changed from the user supplied value, but that's something different. Maybe that's OK, but not sure. It co= uld be that this is a normal technique used to return the number of bytes that = are valid in the user buffer, but I only see that being done in this one functi= on. --=20 You are receiving this mail because: You are the assignee for the bug.=