possible bug in recent crypto patches in master branch

possible bug in recent crypto patches in master branch

[Dorjoy Chowdhury]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

Hi,
I think there maybe some bugs caused by the recent crypto patches that got
merged to master. ref:
https://lore.kernel.org/qemu-devel/CAFEAcA-e_1WFLun2HptTt2bSzXkSMBnxKAK_UzUhwRh_fb66Fg@mail.gmail.com/T/#t

I think before these patches the "qcrypto_hash_bytes" or
"qcrypto_hash_bytesv" apis used to behave such that a user of the apis
could pass his own allocated buffers. In that case, the passed buffers
would be used to fill in the hash result instead of allocating a new
buffer. But I think in the master branch now, these apis always allocate
the result buffer regardless of it's users passing their own buffers or
not. So this is problematic for wherever the users of the apis are passing
their own allocated buffers. For example, I see target/i386/sev.c is one
such api user. Am I missing something here or does this look like it's a
clear bug?

If I am correct, I think it makes sense to keep the old behavior of the
apis where new buffers are not allocated if the user supplies one (I think
it probably also makes sense if we force the users to always provide the
bufffer instead of the apis themselves allocating them). I noticed this bug
in the nitro-enclave work I am doing where rebasing the branch builds but
the actual behavior is buggy because of this new change of the api
implementations.

Regards,
Dorjoy

[-- Attachment #2: Type: text/html, Size: 1594 bytes --]

Re: possible bug in recent crypto patches in master branch

[Daniel P. Berrangé]

On Sun, Oct 13, 2024 at 10:32:36PM +0600, Dorjoy Chowdhury wrote:
> Hi,
> I think there maybe some bugs caused by the recent crypto patches that got
> merged to master. ref:
> https://lore.kernel.org/qemu-devel/CAFEAcA-e_1WFLun2HptTt2bSzXkSMBnxKAK_UzUhwRh_fb66Fg@mail.gmail.com/T/#t
> 
> I think before these patches the "qcrypto_hash_bytes" or
> "qcrypto_hash_bytesv" apis used to behave such that a user of the apis
> could pass his own allocated buffers. In that case, the passed buffers
> would be used to fill in the hash result instead of allocating a new
> buffer. But I think in the master branch now, these apis always allocate
> the result buffer regardless of it's users passing their own buffers or
> not. So this is problematic for wherever the users of the apis are passing
> their own allocated buffers. For example, I see target/i386/sev.c is one
> such api user. Am I missing something here or does this look like it's a
> clear bug?
> 
> If I am correct, I think it makes sense to keep the old behavior of the
> apis where new buffers are not allocated if the user supplies one (I think
> it probably also makes sense if we force the users to always provide the
> bufffer instead of the apis themselves allocating them). I noticed this bug
> in the nitro-enclave work I am doing where rebasing the branch builds but
> the actual behavior is buggy because of this new change of the api
> implementations.

Yes, you're right. The behaviour has changed.

Although the API docs suggested that the 'result' buffer was allocated
internally, they did not explicitly require that, and our impl did
lazy allocation.

I'll work on fixing this and adding test cases for it

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|