| Message ID | f2c8aeb1afefcda92054c448b21fc59cdd99db30.1714360640.git.jeuk20.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,1/1] hw/ufs: Fix buffer overflow bug | expand |
29.04.2024 06:25, Jeuk Kim wrote: > From: Jeuk Kim <jeuk20.kim@samsung.com> > > It fixes the buffer overflow vulnerability in the ufs device. > The bug was detected by sanitizers. > ... > Resolves: #2299 > Fixes: 329f16624499 ("hw/ufs: Support for Query Transfer Requests") > Reported-by: Zheyu Ma <zheyuma97@gmail.com> > Signed-off-by: Jeuk Kim <jeuk20.kim@samsung.com> Cc: qemu-stable@ for 8.2 and 9.0 series. Please do not forget to Cc qemu-stable@ for relevant changes. Thanks, /mjt
On 4/28/24 20:25, Jeuk Kim wrote: > From: Jeuk Kim <jeuk20.kim@samsung.com> > > It fixes the buffer overflow vulnerability in the ufs device. > The bug was detected by sanitizers. > > You can reproduce it by: > > cat << EOF |\ > qemu-system-x86_64 \ > -display none -machine accel=qtest -m 512M -M q35 -nodefaults -drive \ > file=null-co://,if=none,id=disk0 -device ufs,id=ufs_bus -device \ > ufs-lu,drive=disk0,bus=ufs_bus -qtest stdio > outl 0xcf8 0x80000810 > outl 0xcfc 0xe0000000 > outl 0xcf8 0x80000804 > outw 0xcfc 0x06 > write 0xe0000058 0x1 0xa7 > write 0xa 0x1 0x50 > EOF > > Resolves: #2299 > Fixes: 329f16624499 ("hw/ufs: Support for Query Transfer Requests") > Reported-by: Zheyu Ma <zheyuma97@gmail.com> > Signed-off-by: Jeuk Kim <jeuk20.kim@samsung.com> > --- > hw/ufs/ufs.c | 8 ++++++++ > 1 file changed, 8 insertions(+) For some reason this appears to cause failures on s390x: https://gitlab.com/qemu-project/qemu/-/jobs/6740883283 All of the timeouts are new with this patch alone applied, and go away when reverted. I wasn't aware that these tests used ufs, but I have no other explanation... r~
On 30/04/2024 02.17, Richard Henderson wrote: > On 4/28/24 20:25, Jeuk Kim wrote: >> From: Jeuk Kim <jeuk20.kim@samsung.com> >> >> It fixes the buffer overflow vulnerability in the ufs device. >> The bug was detected by sanitizers. >> >> You can reproduce it by: >> >> cat << EOF |\ >> qemu-system-x86_64 \ >> -display none -machine accel=qtest -m 512M -M q35 -nodefaults -drive \ >> file=null-co://,if=none,id=disk0 -device ufs,id=ufs_bus -device \ >> ufs-lu,drive=disk0,bus=ufs_bus -qtest stdio >> outl 0xcf8 0x80000810 >> outl 0xcfc 0xe0000000 >> outl 0xcf8 0x80000804 >> outw 0xcfc 0x06 >> write 0xe0000058 0x1 0xa7 >> write 0xa 0x1 0x50 >> EOF >> >> Resolves: #2299 >> Fixes: 329f16624499 ("hw/ufs: Support for Query Transfer Requests") >> Reported-by: Zheyu Ma <zheyuma97@gmail.com> >> Signed-off-by: Jeuk Kim <jeuk20.kim@samsung.com> >> --- >> hw/ufs/ufs.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) > > For some reason this appears to cause failures on s390x: > > https://gitlab.com/qemu-project/qemu/-/jobs/6740883283 > > All of the timeouts are new with this patch alone applied, > and go away when reverted. > > I wasn't aware that these tests used ufs, but I have no > other explanation... I don't know for sure, but the test failure might instead be related to the problem that gets fixed by https://lore.kernel.org/qemu-devel/20240429075908.36302-1-thuth@redhat.com/ ... I'm preparing a pull request for that fix right now, so maybe you could try this ufs pull request afterwards again to see whether the problem is fixed? Thomas
On 30/04/2024 06.32, Thomas Huth wrote: > On 30/04/2024 02.17, Richard Henderson wrote: >> On 4/28/24 20:25, Jeuk Kim wrote: >>> From: Jeuk Kim <jeuk20.kim@samsung.com> >>> >>> It fixes the buffer overflow vulnerability in the ufs device. >>> The bug was detected by sanitizers. >>> >>> You can reproduce it by: >>> >>> cat << EOF |\ >>> qemu-system-x86_64 \ >>> -display none -machine accel=qtest -m 512M -M q35 -nodefaults -drive \ >>> file=null-co://,if=none,id=disk0 -device ufs,id=ufs_bus -device \ >>> ufs-lu,drive=disk0,bus=ufs_bus -qtest stdio >>> outl 0xcf8 0x80000810 >>> outl 0xcfc 0xe0000000 >>> outl 0xcf8 0x80000804 >>> outw 0xcfc 0x06 >>> write 0xe0000058 0x1 0xa7 >>> write 0xa 0x1 0x50 >>> EOF >>> >>> Resolves: #2299 >>> Fixes: 329f16624499 ("hw/ufs: Support for Query Transfer Requests") >>> Reported-by: Zheyu Ma <zheyuma97@gmail.com> >>> Signed-off-by: Jeuk Kim <jeuk20.kim@samsung.com> >>> --- >>> hw/ufs/ufs.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >> >> For some reason this appears to cause failures on s390x: >> >> https://gitlab.com/qemu-project/qemu/-/jobs/6740883283 >> >> All of the timeouts are new with this patch alone applied, >> and go away when reverted. >> >> I wasn't aware that these tests used ufs, but I have no >> other explanation... > > I don't know for sure, but the test failure might instead be related to the > problem that gets fixed by > https://lore.kernel.org/qemu-devel/20240429075908.36302-1-thuth@redhat.com/ > ... I'm preparing a pull request for that fix right now, so maybe you could > try this ufs pull request afterwards again to see whether the problem is fixed? Hmm, thinking about it twice, it cannot be the reason: That bug affects aarch64/arm only, and in above CI run, some other targets were failing. So the problem must be something else, indeed. Thomas
diff --git a/hw/ufs/ufs.c b/hw/ufs/ufs.c index eccdb852a0..bac78a32bb 100644 --- a/hw/ufs/ufs.c +++ b/hw/ufs/ufs.c @@ -126,6 +126,10 @@ static MemTxResult ufs_dma_read_req_upiu(UfsRequest *req) copy_size = sizeof(UtpUpiuHeader) + UFS_TRANSACTION_SPECIFIC_FIELD_SIZE + data_segment_length; + if (copy_size > sizeof(req->req_upiu)) { + copy_size = sizeof(req->req_upiu); + } + ret = ufs_addr_read(u, req_upiu_base_addr, &req->req_upiu, copy_size); if (ret) { trace_ufs_err_dma_read_req_upiu(req->slot, req_upiu_base_addr); @@ -225,6 +229,10 @@ static MemTxResult ufs_dma_write_rsp_upiu(UfsRequest *req) copy_size = rsp_upiu_byte_len; } + if (copy_size > sizeof(req->rsp_upiu)) { + copy_size = sizeof(req->rsp_upiu); + } + ret = ufs_addr_write(u, rsp_upiu_base_addr, &req->rsp_upiu, copy_size); if (ret) { trace_ufs_err_dma_write_rsp_upiu(req->slot, rsp_upiu_base_addr);