| Message ID | 20210425173353.10231-1-mail@anirudhrb.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6be388f4a35d2ce5ef7dbf635a8964a5da7f799f |
| Headers | show |
| Series | usbhid: fix info leak in hid_submit_ctrl | expand |
On Sun, 25 Apr 2021, Anirudh Rayabharam wrote: > In hid_submit_ctrl(), the way of calculating the report length doesn't > take into account that report->size can be zero. When running the > syzkaller reproducer, a report of size 0 causes hid_submit_ctrl) to > calculate transfer_buffer_length as 16384. When this urb is passed to > the usb core layer, KMSAN reports an info leak of 16384 bytes. > > To fix this, first modify hid_report_len() to account for the zero > report size case by using DIV_ROUND_UP for the division. Then, call it > from hid_submit_ctrl(). > > Reported-by: syzbot+7c2bb71996f95a82524c@syzkaller.appspotmail.com > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> Benjamin, could you please run this one through your regression testing machinery before we send it upstream? Thanks,
On Wed, May 5, 2021 at 2:42 PM Jiri Kosina <jikos@kernel.org> wrote: > > On Sun, 25 Apr 2021, Anirudh Rayabharam wrote: > > > In hid_submit_ctrl(), the way of calculating the report length doesn't > > take into account that report->size can be zero. When running the > > syzkaller reproducer, a report of size 0 causes hid_submit_ctrl) to > > calculate transfer_buffer_length as 16384. When this urb is passed to > > the usb core layer, KMSAN reports an info leak of 16384 bytes. > > > > To fix this, first modify hid_report_len() to account for the zero > > report size case by using DIV_ROUND_UP for the division. Then, call it > > from hid_submit_ctrl(). > > > > Reported-by: syzbot+7c2bb71996f95a82524c@syzkaller.appspotmail.com > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > Benjamin, could you please run this one through your regression testing > machinery before we send it upstream? > I don't have a reproducer like syzbot has for the exact bug here, as I am relying on one real USB device to check if usbhid is not too broken. However, the test suite should catch if there is an error implied by the hid_report_len() change. Anyway, I manually started the job and will report when it is done. Cheers, Benjamin
On Wed, 5 May 2021, Benjamin Tissoires wrote: > I don't have a reproducer like syzbot has for the exact bug here, as I > am relying on one real USB device to check if usbhid is not too broken. > However, the test suite should catch if there is an error implied by the > hid_report_len() change. Yes, that was exactly what I wanted to check, sorry for not being verbose enough :) > Anyway, I manually started the job and will report when it is done. Thanks,
On Wed, May 5, 2021 at 3:28 PM Jiri Kosina <jikos@kernel.org> wrote: > > On Wed, 5 May 2021, Benjamin Tissoires wrote: > > > I don't have a reproducer like syzbot has for the exact bug here, as I > > am relying on one real USB device to check if usbhid is not too broken. > > However, the test suite should catch if there is an error implied by the > > hid_report_len() change. > > Yes, that was exactly what I wanted to check, sorry for not being verbose > enough :) > > > Anyway, I manually started the job and will report when it is done. > Heh, no problems. "Job succeeded" \o/ Given that you are on a spree: Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cheers, Benjamin
On Wed, 5 May 2021, Benjamin Tissoires wrote: > > > I don't have a reproducer like syzbot has for the exact bug here, as I > > > am relying on one real USB device to check if usbhid is not too broken. > > > However, the test suite should catch if there is an error implied by the > > > hid_report_len() change. > > > > Yes, that was exactly what I wanted to check, sorry for not being verbose > > enough :) > > > > > Anyway, I manually started the job and will report when it is done. > > > > Heh, no problems. > > "Job succeeded" \o/ > > Given that you are on a spree: :-) > Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Applied to for-5.13/upstream-fixes. Thanks,
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 86257ce6d619..4e9077363c96 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -374,7 +374,7 @@ static int hid_submit_ctrl(struct hid_device *hid) raw_report = usbhid->ctrl[usbhid->ctrltail].raw_report; dir = usbhid->ctrl[usbhid->ctrltail].dir; - len = ((report->size - 1) >> 3) + 1 + (report->id > 0); + len = hid_report_len(report); if (dir == USB_DIR_OUT) { usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0); usbhid->urbctrl->transfer_buffer_length = len; diff --git a/include/linux/hid.h b/include/linux/hid.h index 271021e20a3f..10e922cee4eb 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -1167,8 +1167,7 @@ static inline void hid_hw_wait(struct hid_device *hdev) */ static inline u32 hid_report_len(struct hid_report *report) { - /* equivalent to DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) */ - return ((report->size - 1) >> 3) + 1 + (report->id > 0); + return DIV_ROUND_UP(report->size, 8) + (report->id > 0); } int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
In hid_submit_ctrl(), the way of calculating the report length doesn't take into account that report->size can be zero. When running the syzkaller reproducer, a report of size 0 causes hid_submit_ctrl) to calculate transfer_buffer_length as 16384. When this urb is passed to the usb core layer, KMSAN reports an info leak of 16384 bytes. To fix this, first modify hid_report_len() to account for the zero report size case by using DIV_ROUND_UP for the division. Then, call it from hid_submit_ctrl(). Reported-by: syzbot+7c2bb71996f95a82524c@syzkaller.appspotmail.com Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> --- drivers/hid/usbhid/hid-core.c | 2 +- include/linux/hid.h | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)