Incorrect handling of 0x46 CDB commands (ATAPI)

[disean]

Version: 08c8ed9
ISO File: https://github.com/reactos/reactos/actions/runs/19678484187/artifacts/4676568686 (livecd.iso)

When the host sends a 0x46 CDB command, which has allocation length of 8, bochs incorrectly returns more than 8 bytes of data, resulting in a host buffer overrun.

Send packet command
    IO 000001F6 <-- a0
    IO 000001F7 --> 50
    IO 000001F4 <-- 08
    IO 000001F5 <-- 00
    IO 000001F1 <-- 00
    IO 000001F7 <-- a0
    
Wait for ready for CDB
    IO 000001F7 --> 58
    IO 000001F2 --> 01 (IR = 09, phase = accept CDB)

Send CDB 46 (data read)
    IO 000001F0 <-- 00000046 Cdb[0] I/O 32
    IO 000001F0 <-- 00000000 Cdb[1] I/O 32
    IO 000001F0 <-- 00000008 Cdb[2] I/O 32 (allocation length = 8)
    
Interrupt
    IO 0000C002 --> 04
    IO 0000C002 <-- 04 (clear BM DMA)
    
    IO 000001F7 --> 58
    IO 000001F2 --> 02 (IR = 0A, phase = data read)
    
    IO 000001F4 --> 08
    IO 000001F5 --> 00 (data size = 8 bytes)

Read data (8 bytes)
    IO 000001F0 --> 48000000 Cdb[0] I/O 32
    IO 000001F0 --> 08000000 Cdb[1] I/O 32

Interrupt
    IO 0000C002 --> 04
    IO 0000C002 <-- 04 (clear BM DMA)
    
    Incorrect phase transition goes here:
-   IO 000001F7 --> 58
-   IO 000001F2 --> 02 (IR = 0A, phase = data read)

-   IO 000001F4 --> 08
-   IO 000001F5 --> 00 (data size = 8 bytes)
    
    Expected behavior:
+   IO 000001F7 --> 50
+   IO 000001F2 --> 03 (IR = 03, phase = status)

[fysnet]

The following comment (from me) probably proves your idea:

Bochs/bochs/iodev/harddrv.cc

Lines 2079 to 2081 in 08c8ed9

	// I think the last parameter needs to be 'alloc_length', but ReactOS won't boot unless it is this:
	init_send_atapi_command(channel, atapi_command, return_length + 4, return_length + 4);

Since ReactOS has quite a few errors, I believe we should make this correct instead of a keeping a hack to allow ReactOS to work.

A fix might simply be:

-  init_send_atapi_command(channel, atapi_command, return_length + 4, return_length + 4);
+  init_send_atapi_command(channel, atapi_command, return_length + 4, alloc_length);

[fysnet]

Were you able to make the change I suggested and test it again?

I failed to notice that your test was actually ReactOS. Does this mean that there has been a fix since I tested command 0x46 a few months ago, which at the time, required this error in Bochs to even boot up?

If I get a chance, I will test the image you provide as well.

[disean]

Were you able to make the change I suggested and test it again?

I need to work out how to build bochs on Windows.

Does this mean that there has been a fix since I tested command 0x46 a few months ago, which at the time, required this error in Bochs to even boot up?

Nope, I ran ReactOS with an out-of-tree driver for PCI IDE controllers in bochs.

[fysnet]

Please check with #683.

If you create a fork of Bochs, you can make changes and have online builds created. For example, if you go to my fork, and click on the Actions 'tab' ( https://github.com/fysnet/Bochs-USB-Edition/actions ), you can download one of two Windows builds.

If you setup your fork to use the Actions tab, you can then do the same. No need to have a local build environment. Great if you plan to only make minor changes.

[disean]

Yes, it actually fixes the issue, but that breaks a stock ReactOS build.

None of the test suites I have (WinXP, etc.) call this command, so I don't have any test cases, though I think this fix is correct.

You might want to test the change on anything newer than NT5.x. Modern storage class drivers issue this command during initialization. https://github.com/microsoft/Windows-driver-samples/blob/96eb96dfb613e4c745db6bd1f53a92fe7e2290fc/storage/class/cdrom/src/mmc.c#L625-L630

[fysnet]

Yes, it actually fixes the issue, but that breaks a stock ReactOS build.

Yep, hence the reason for the comment within the source and the hack. :-)

None of the test suites I have (WinXP, etc.) call this command, so I don't have any test cases, though I think this fix is correct.

I do have a Win7 and Win8 simulation, I will just have to update the bochsrc.txt files to match the recent changes (I haven't tried these two simulations for a few years now).

As for the PR, I think PR #683 should be applied. I don't think we should cater to ReactOS' inability to boot by hacking Bochs. The PR leaves the comment in for future reference, so that if this issue ever comes up again, we know what to do.

Thanks for verifying the fix.

[Vort]

I don't think we should cater to ReactOS' inability to boot by hacking Bochs.

Does it fail to boot with real hardware as well?
If not, then what are the differences between emulation and real hardware and why they exist?

[fysnet]

Does it fail to boot with real hardware as well? If not, then what are the differences between emulation and real hardware and why they exist?

I currently don't have a 32-bit machine with a working cd-rom. If it is a must, I could probably go through my storage and find a working cd-rom and a 32-bit machine and try, but I don't think it is worth the trouble for this particular issue. I believe, with very little doubt, that the PR is correct and the current code (before the PR) is a hack to make ReactOS boot. Just my opinion though :-)

[obsolete-is-better]

I don't think we should cater to ReactOS' inability to boot by hacking Bochs.

Does it fail to boot with real hardware as well? If not, then what are the differences between emulation and real hardware and why they exist?

i've been able to boot both the ReactOS ATA PR and official current builds on real hardware no crashes.
although to be sure i guess i would probably have to test Intel 440BX hardware specifically?

[Vort]

Regarding differences between behavior with emulated and real hardware:
Long time ago I saw checks in ReactOS code like if (Bochs) ....
So it may be that ReactOS assumes Bochs will never change (and never fix bugs).

but I don't think it is worth the trouble for this particular issue

With exception of situation described above and several other unlucky coincidences, difference in behavior means there is a bug in Bochs.
It may be ok to merge change, which looks more correct, and don't investigate problem further, but having 100% reproduction is a nice opportunity, other starting points for bug fixing are usually worse.

[obsolete-is-better]

I have tested a recent official ReactOS build on real 440BX hardware and the ATA PR build both boot from CD successfully.

ATA PR debug log:
ReactOS 0.4.16-x86-dev (Build 20251125-076ff8c).txt

official build:
ReactOS 0.4.16-x86-dev (Build 20251201-0.4.16-dev-1945-g054b8e0).txt

so it seems this is a emulation bug in Bochs.

[fysnet]

I believe the difference in your actual hardware boot and Bochs boot is the way the data is transferred.
In your actual hardware, it uses PRD's

PCIIDEX: (pdo.c:81) PciIdeXPdoInitDma CH 0: Allocated 33 PRD pages, maximum transfer length 0x20000

where it has allocated 0x20000 bytes for the transfer. This will never overrun with a 0x46 call.

In the Bochs simulation, ReactOS is using Port I/O, a word at a time (as opposed to DMA as in above), which can (and does in this case) overrun with a 0x46 call.

So, is this a bug in Bochs? Not with the 0x46 command emulation.

However, it could be somewhere else where ReactOS doesn't recognize that it can use PRDs while within Bochs, whether this is a ReactOS issue or a Bochs issue, it will be a further investigation.