From owner-freebsd-bugs@freebsd.org Thu May 25 12:02:42 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 B6B90D78CB3 for ; Thu, 25 May 2017 12:02:42 +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 A6CD21D8F for ; Thu, 25 May 2017 12:02:42 +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 v4PC2g0m088178 for ; Thu, 25 May 2017 12:02:42 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: Thu, 25 May 2017 12:02:42 +0000 X-Bugzilla-Reason: AssignedTo X-Bugzilla-Type: new 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: ecturt@gmail.com 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: bug_id short_desc product version rep_platform op_sys bug_status bug_severity priority component assigned_to reporter Message-ID: 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: Thu, 25 May 2017 12:02:42 -0000 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=3D219525 Bug ID: 219525 Summary: Multiple bugs in mpr ioctl handler Product: Base System Version: CURRENT Hardware: Any OS: Any Status: New Severity: Affects Only Me Priority: --- Component: kern Assignee: freebsd-bugs@FreeBSD.org Reporter: ecturt@gmail.com There are multiple bugs in the `mpr` device's ioctl handler, `mpr_ioctl`. Multiple `malloc` calls are made with unlimited, user controlled, unsigned 32bit integers and the `M_WAITOK` flag, meaning that if a large enough requ= est is made, it can crash the system. `M_WAITOK` should not be passed, and the = call should return `ENOMEM` if `malloc` failed (along with any necessary cleanup= ). case MPRIO_READ_CFG_PAGE: mpr_page =3D malloc(page_req->len, M_MPRUSER, M_WAITOK | M_ZERO); ... case MPRIO_READ_EXT_CFG_PAGE: mpr_page =3D malloc(ext_page_req->len, M_MPRUSER, M_WAITOK | M_ZERO); ... case MPRIO_WRITE_CFG_PAGE: mpr_page =3D malloc(page_req->len, M_MPRUSER, M_WAITOK|M_ZERO); The `MPTIOCTL_PASS_THRU` command leads to a `copyin` call with user supplie= d, unchecked, unsigned 32bit integer, and destination a struct on the stack. Leading to stack overflow with arbitrary contents and size. The copy size h= ere should be `sizeof(tmphdr)` instead. This command then performs some validation on `data->RequestSize`, but at t= his point `mpr_lock` is not held, and so `IOCRequestFrameSize` value can change, and this check can be raced, if for example a Diag Reset is made in another thread (ioctl command `MPTIOCTL_RESET_ADAPTER` can do this for example). Th= is same bug is repeated in ioctl command `MPRIO_MPR_COMMAND`. A `copyout` call can be made with user supplied unsigned 32bit integer `ReplySize`. The code checks if `sz > data->ReplySize`, but doesn't check if `sz < data->ReplySize`. Results in copying out of bounds memory to userland. The header copied in from userland earlier is then copied to task, and uses `data->RequestSize`, which should just be the size of the source struct, `sizeof(tmphdr)`. This happens again later on as well. Another `malloc` call is made with unlimited, user controlled, unsigned 32b= it integer and `M_WAITOK` flag. case MPTIOCTL_PASS_THRU: /* * The user has requested to pass through a command to be * executed by the MPT firmware. Call our routine which does * this. Only allow one passthru IOCTL at one time. */ error =3D mpr_user_pass_thru(sc, (mpr_pass_thru_t *)arg); break; static int mpr_user_pass_thru(struct mpr_softc *sc, mpr_pass_thru_t *data) { MPI2_REQUEST_HEADER *hdr, tmphdr;=20=20=20=20 ... /* * copy in the header so we know what we're dealing with before we * commit to allocating a command for it. */ err =3D copyin(PTRIN(data->PtrRequest), &tmphdr, data->RequestSize); if (err !=3D 0) goto RetFreeUnlocked; if (data->RequestSize > (int)sc->facts->IOCRequestFrameSize * 4) { err =3D EINVAL; goto RetFreeUnlocked; ... if (sz > data->ReplySize) { mpr_printf(sc, "%s: user reply buffer (%d) " "smaller than returned buffer (%d)\n", __func__, data->ReplySize, sz); } mpr_unlock(sc); copyout(cm->cm_reply, PTRIN(data->PtrReply), data->ReplySize); mpr_lock(sc); ... /* Copy the header in. Only a small fixup is needed. */ task =3D (MPI2_SCSI_TASK_MANAGE_REQUEST *)cm->cm_req; bcopy(&tmphdr, task, data->RequestSize); ... hdr =3D (MPI2_REQUEST_HEADER *)cm->cm_req; bcopy(&tmphdr, hdr, data->RequestSize); ... cm->cm_length =3D MAX(data->DataSize, data->DataOutSize); cm->cm_out_len =3D data->DataOutSize; cm->cm_flags =3D 0; if (cm->cm_length !=3D 0) { cm->cm_data =3D malloc(cm->cm_length, M_MPRUSER, M_WAITOK | M_ZERO); ... } The `MPTIOCTL_EVENT_REPORT` command calls `mpr_user_event_report`, which incorrectly checks user supplied unsigned 32bit size. 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)`. case MPTIOCTL_EVENT_REPORT: /* * The user has done an event report. Call our routine which * does this. */ error =3D mpr_user_event_report(sc, (mpr_event_report_t *)arg); break; static int mpr_user_event_report(struct mpr_softc *sc, mpr_event_report_t *data) { int status =3D 0; uint32_t size; 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); return (status); } The `MPTIOCTL_REG_ACCESS` command doesn't validate user supplied, unsigned 32bit `RegOffset` integer. This command leads to `bus_space_write_4` with arbitrary offset. From the man page: "The bus_space_write_N() family of functions writes a 1, 2, 4, or 8 byte data item to the offset specified by offset into the region specified by handle of the bus space specified by sp= ace. The location being written must lie within the bus space region specified = by handle." case MPTIOCTL_REG_ACCESS: /* * The user has requested register access. Call our routine * which does this. */ mpr_lock(sc); error =3D mpr_user_reg_access(sc, (mpr_reg_access_t *)arg); mpr_unlock(sc); break; static int mpr_user_reg_access(struct mpr_softc *sc, mpr_reg_access_t *data) { int status =3D 0; switch (data->Command) { ... case REG_MEM_READ: data->RegData =3D mpr_regread(sc, data->RegOffset); break; case REG_MEM_WRITE: mpr_regwrite(sc, data->RegOffset, data->RegData); break; ... } return (status); } ... static __inline void mpr_regwrite(struct mpr_softc *sc, uint32_t offset, uint32_t val) { bus_space_write_4(sc->mpr_btag, sc->mpr_bhandle, offset, val); } The `MPTIOCTL_BTDH_MAPPING` command leads to an off-by-one error in `mpr_user_btdh`. case MPTIOCTL_BTDH_MAPPING: /* * The user has requested to translate a bus/target to a * DevHandle or a DevHandle to a bus/target. Call our routine * which does this. */ error =3D mpr_user_btdh(sc, (mpr_btdh_mapping_t *)arg); break; static int mpr_user_btdh(struct mpr_softc *sc, mpr_btdh_mapping_t *data) { uint8_t bt2dh =3D FALSE; uint8_t dh2bt =3D FALSE; uint16_t dev_handle, bus, target; bus =3D data->Bus; target =3D data->TargetID; dev_handle =3D data->DevHandle; ... if (target > sc->max_devices) { mpr_dprint(sc, MPR_XINFO, "Target ID is out of range " "for Bus/Target to DevHandle mapping."); return (EINVAL); } dev_handle =3D sc->mapping_table[target].dev_handle; ... return (0); } The check for `target` is that an error should be returned if `target > sc->max_devices`, however this check should be changed to `target >=3D sc->max_devices`, since if `target` is equal to `max_devices` it will be an= out of bounds access, since `mapping_table` is allocated with `max_devices` elements: int mpr_mapping_allocate_memory(struct mpr_softc *sc) { uint32_t dpm_pg0_sz; sc->mapping_table =3D malloc((sizeof(struct dev_mapping_table) * sc->max_devices), M_MPR, M_ZERO|M_NOWAIT); ... --=20 You are receiving this mail because: You are the assignee for the bug.=