Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vendor defined request size #2666

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

m-milinkovic
Copy link
Contributor

The pointer of Vendor Defined Request is moved, then the request size should be adjusted accordingly.

@steven-bellock
Copy link
Contributor

@m-milinkovic you'll need to replace the whole copyright header with

/**
 *  Copyright Notice:
 *  Copyright 2023-2024 DMTF. All rights reserved.
 *  License: BSD 3-Clause License. For full text see link: https://github.com/DMTF/libspdm/blob/main/LICENSE.md
 **/

For some reason this file doesn't have the Copyright Notice: line.

@m-milinkovic m-milinkovic force-pushed the fix-vendor-defined-request-size branch from 8e67dbe to 409517d Compare April 19, 2024 16:29
@jyao1
Copy link
Member

jyao1 commented Apr 22, 2024

@uvinrg, could you please take a look?

Copy link
Member

@jyao1 jyao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-milinkovic, I think below calculation is risky, since it includes the transport padding.

    req_size = (uint16_t)request_size -
               sizeof(spdm_vendor_defined_request_msg_t) -
               ((const spdm_vendor_defined_request_msg_t*)request)->len -
               sizeof(uint16_t);

The req_size should be got from 2 bytes field ReqLength in spdm_request, which is precise size. See below:

    req_vendor_id = ((const uint8_t *)request) +
                    sizeof(spdm_vendor_defined_request_msg_t);
    req_size = *(const uint16_t *)(req_vendor_id + spdm_request->len);

The pointer of Vendor Defined Request is moved, then the request size should be adjusted accordingly.

Signed-off-by: Milan Milinkovic <[email protected]>
@m-milinkovic m-milinkovic force-pushed the fix-vendor-defined-request-size branch from 409517d to 162cf02 Compare April 24, 2024 17:39
@m-milinkovic
Copy link
Contributor Author

m-milinkovic commented Apr 24, 2024

@m-milinkovic, I think below calculation is risky, since it includes the transport padding.

    req_size = (uint16_t)request_size -
               sizeof(spdm_vendor_defined_request_msg_t) -
               ((const spdm_vendor_defined_request_msg_t*)request)->len -
               sizeof(uint16_t);

The req_size should be got from 2 bytes field ReqLength in spdm_request, which is precise size. See below:

    req_vendor_id = ((const uint8_t *)request) +
                    sizeof(spdm_vendor_defined_request_msg_t);
    req_size = *(const uint16_t *)(req_vendor_id + spdm_request->len);

Done.
Thank you for the review!

Please note that req_data could be obtained in a similar manner (via req_vendor_id), but I already made this commit a bit dirty by renaming spdm_vendor_defined_response_msg_t to spdm_vendor_defined_request_msg_t. I prefer not to go beyond that trivial change.

@m-milinkovic
Copy link
Contributor Author

Also, please note that, if you decide to merge this commit, I'll check other affected repos (e.g., SPDM-Responder-Validator) and propose a solution as well.

@jyao1 jyao1 merged commit dfe8bee into DMTF:main Apr 24, 2024
97 checks passed
@uvinrg
Copy link
Contributor

uvinrg commented Apr 25, 2024

Hi guys, sorry for the late response. Thank you for the fix, looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants