Fix user data alignment in MEMBLOCK

[mbikovitsky]

Kernel memory allocations on Windows should be aligned on MEMORY_ALLOCATION_ALIGNMENT (16 bytes on x64 and 8 bytes on x86).

[aquynh]

cool! with this patch, Windows kernel driver works well now?

[mbikovitsky]

It depends.

For one, the current implementation returns 4-byte aligned memory on x86 (due to the size_t field just before the data array), which means that allocations that require larger alignment (for instance, doubles) are silently broken. On x86 and x64 it is usually not a problem, since these processors are rather forgiving of alignment issues (and according to this, ARM64 is fine as well, though the article refers to user-mode code only). Still, code that uses this memory assumes it is correctly aligned, and may crash if the compiler generated some alignment-sensitive instructions.

On Windows, the kernel memory allocation APIs all return memory that is aligned at least on MEMORY_ALLOCATION_ALIGNMENT, and so it is only logical to do the same. There are actually some data structures that require the 16-byte alignment.

The point is that while the current implementation probably works fine, it is still worth fixing. I don't know whether the driver will work well now, but it will certainly work better :)

[aquynh]

@tandasat, can you please ack?

[tandasat]

I’m traveling now. I’ll check it by the end of this weekend.

[aquynh]

@tandasat, at the same time, please could you confirm that in the "v4" branch, your Windows driver still compiles without any issues?

thanks!

[tandasat]

Will do. If you have the issue for this already, let me know so I can quickly catch up problems if any.

[tandasat]

@mbikovitsky @aquynh

I notice there are two implementation of windows driver support:
capstone\contrib\windows_kernel, and
capstone\windows

Is the first one usable? When I merged the second one, it did not even compile (and so I had to create the 2nd one). I ask this because the patch is for the first one, and I am thinking of deprecating or deleting it and consolidate Windows kernel support implementation into one. Thoughts?

Note that the patch itself looks reasonable. except that it should be done in capstone\windows\winkernel_mm.c (this is why we want to deprecate one).

[mbikovitsky]

Now that I look more closely at it, the version in contrib uses std::unique_ptr which is not available for Windows drivers (as is the rest of the C++ standard library). Apart from that, both implementations are very similar with regards to memory allocations. The only major difference I see is in the vsnprintf implementation.

I pushed a commit with the same change in winkernel_mm.c.

[tandasat]

Thank you @mbikovitsky. The changes look good to me.

@aquynh not to block a merge of this PR but want to hear your thoughts on deleting capstone\contrib\windows_kernel from the v4 branch.

[aquynh]

the one under contrib\ is from @zer0mem, but i am not sure he still wants to maintain it.

lets go ahead with this for now.

[aquynh]

so i will just merge this?

[tandasat]

Yes, let’s merge this.

[aquynh]

merged for all branches "master", "v4" & "next", thanks!

@tandasat so you confirmed that all builds without any issues on "v4" branch?

(i want to release v4.0.2 based on "v4" branch next week)

[tandasat]

I have not been able to check that yet. I’ll give you an update by the end of this weekend.

[tandasat]

I am working on windows-kernel support for v4 in this issue: #1474