-
Notifications
You must be signed in to change notification settings - Fork 153
qcbor: library to expose Qualcomm Security Package #1166
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
Conversation
|
Hi reviewer, this PR has been separated from PR1094. Following our internal discussion, we have decided to share the qcbor, qcom-teec, and minkipc components individually. |
recipes-security/qcbor/qcbor_git.bb
Outdated
| HOMEPAGE = "https://github.com/laurencelundblade/QCBOR.git" | ||
| SECTION = "libs" | ||
|
|
||
| LICENSE = "BSD-3-Clause" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qcbor is using the BSD-3-clause license:
ref: https://github.com/laurencelundblade/QCBOR?tab=License-1-ov-file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it what is stated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BSD 3-Clause License
Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are met:
-
Redistributions of source code must retain the above copyright notice, this
list of conditions and the following disclaimer. -
Redistributions in binary form must reproduce the above copyright notice,
this list of conditions and the following disclaimer in the documentation
and/or other materials provided with the distribution. -
Neither the name of the copyright holder nor the names of its
contributors may be used to endorse or promote products derived from
this software without specific prior written permission.
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
recipes-security/qcbor/qcbor_git.bb
Outdated
| SRCREV = "7d9f0b787150d739ab50805008bc7142bc9b7822" | ||
| PV = "0.0+git" | ||
|
|
||
| EXTRA_OECMAKE = " -DBUILD_QCBOR_TEST=APP" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is use of -DBUILD_QCBOR_TEST ? enabling this flag means you are generating a unit test client too along with library ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If intention is to build test application along with the libs, use a PACKAGECONFIG check and use the same in installing the test bin as well as to add ${PN}-test to PACAKGES. Otherwise this recipe will end up with an empty test package and do_install failures when user wants to drop BUILD_QCBOR_TEST from OECMAKE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question of using PACKAGECONFIG is orthogonal. No, the user will not end up with an empty package, OE handles it internally. None of the recipes I know check PACKAGECONFIG when populating PACKAGES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If intention is to build test application along with the libs, use a PACKAGECONFIG check and use the same in installing the test bin as well as to add ${PN}-test to PACAKGES. Otherwise this recipe will end up with an empty test package and do_install failures when user wants to drop BUILD_QCBOR_TEST from OECMAKE.
Updated. PACKAGECONFIG is a boolean variable, so because we have three options (OFF / LIB / APP), we created a custom variable called QCBOR_TEST_MODE.
recipes-security/qcbor/qcbor_git.bb
Outdated
| install -m 0755 ${WORKDIR}/build/test/qcbortest ${D}${bindir}/ | ||
| } | ||
|
|
||
| PACKAGES += "${PN}-test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in which package the qcbor.so going ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lumag , reviewers, what is the recommended way to include qcbor in the image? The build command kas build meta-qcom/ci/qcs6490-rb3gen2-core-kit.yml compiles the core-image-base target, which is defined in the OE-core layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the library or the test utils?
For the library you don't need to "include" it. Let it be pulled in by the dependencies. For the test utils, please include it into the test image. You had a nice idea of a security-testing packagegroup. I don't understand, why you dropped it .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I indecated the test utils. How can we include the qcbortest into the .img file and flash it onto the device?
In this pull request, qcbor is not required as a dependency, so the packagegroup-secure package has been removed to enable faster code merging, we will add this package in the Minkipc pull request.
recipes-security/qcbor/qcbor_git.bb
Outdated
|
|
||
| do_install:append() { | ||
| install -d ${D}${bindir} | ||
| install -m 0755 ${WORKDIR}/build/test/qcbortest ${D}${bindir}/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please patch the CMakeList to use make install instead. Send the patch upstream
recipes-security/qcbor/qcbor_git.bb
Outdated
| install -m 0755 ${WORKDIR}/build/test/qcbortest ${D}${bindir}/ | ||
| } | ||
|
|
||
| PACKAGES += "${PN}-test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What goes to this package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
FILES:${PN}-ptest += " ${bindir}/qcbortest"
recipes-security/qcbor/qcbor_git.bb
Outdated
|
|
||
| PACKAGES += "${PN}-test" | ||
|
|
||
| FILES:${PN}-bin += " ${bindir}/qcbortest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package is not defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
FILES:${PN}-ptest += " ${bindir}/qcbortest"
lumag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One the top of that, please write an appropriate commit subject and message. Please use git log to get some examples.
recipes-security/qcbor/qcbor_git.bb
Outdated
| git://github.com/laurencelundblade/QCBOR.git;branch=master;protocol=https \ | ||
| " | ||
| SRCREV = "7d9f0b787150d739ab50805008bc7142bc9b7822" | ||
| PV = "0.0+git" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is plain wrong. It is not 0.0. There was a 1.8.3 release. Please use the release instead of packaging random git commit (and lying about the version).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, Have updated the version as 1.5.3.
ref: https://github.com/laurencelundblade/QCBOR/releases
PV = "1.5.3+git"
4df802f to
2947395
Compare
recipes-security/qcbor/qcbor_git.bb
Outdated
| PV = "0.0+git" | ||
|
|
||
| # QCBOR_TEST_MODE:OFF / LIB / APP | ||
| QCBOR_TEST_MODE ?= "APP" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? Why is it selectable?
recipes-security/qcbor/qcbor_git.bb
Outdated
| EXTRA_OECMAKE += "-DBUILD_QCBOR_TEST=${QCBOR_TEST_MODE}" | ||
|
|
||
| do_install:append() { | ||
| if [ "${QCBOR_TEST_MODE}" = "APP" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring review will bring you nowhere. Fold this into make install.
recipes-security/qcbor/qcbor_git.bb
Outdated
| @@ -0,0 +1,35 @@ | |||
| SUMMARY = "Qualcomm QCBOR library" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Qualcomm? It is just the upstream project.
|
There is already a qcbor recipe in meta-oe, can't we just use that one instead (and update to a newer rev if required)? https://git.openembedded.org/meta-openembedded/tree/meta-oe/recipes-extended/qcbor/qcbor_1.5.3.bb |
2947395 to
a4ac41c
Compare
Yes, the QCbor BB file is already present in the EO layer. However, we do not manage meta-EO. If a new version of QCbor is required in meta-QCOM later on, we will need to update it in meta-EO, but the timing for this is uncertain. Additionally, when compiling qcbor with OE, only the library is produced, and qcbortest is not generated. This will make debugging qcbor challenging for future products that use the meta-qcom branch. |
|
Hi @lumag , @ricardosalveti, @vksharma-oss, @vkraleti , many thanks for your review and support. |
Well this is not at all correct and right. Doing a parallel development for open source project can lead to fragmentation, long-term maintenance challenges and may end up in Frankenstein. We should first prioritize reusing the existing recipe in meta-oe. If enhancements are needed, they should be submitted to upstream meta-oe, get them reviewed and merged. During reviews, if some challenges come up due to misalignment with community then only we should consider adding a bbappend in meta-qcom. Without even attempting to upstream, completely replacing the recipe is not an acceptable solution. |
Hi @vkraleti @lumag @ricardosalveti Build command:
|
|
Being a BSP layer, meta-qcom is expected to be built standalone using oe-core without dependency on any other layers like meta-oe. If QTEE has dependencies on recipes from meta-oe, may be it can be added to dynamic-layers section to conditionally compile. |
Right, let's use it via dynamic-layers for now until we can validate the workflow and the builds, later on we can either propose for oe-core (harder in this scenario as we should have at least a common dependency there), or bring the recipe as part of meta-qcom but syncing it with the recipe available under meta-oe. What I really want to avoid is multiple versions and variants of the same recipe in multiple layers, because meta-oe is also available with meta-qcom-distro. |
Add a recipe to build qcbor, library to work with security functionality on Qualcomm platforms. The libqcbor, which are only available through meta-oe, is depended by Qcom-teec and MinkIPC. Thus add libqcbor recipe to the corresponding dynamic-layers subdir. Signed-off-by: Jiaxing Li <[email protected]>
a4ac41c to
fbacb11
Compare
Hi @lumag , @vkraleti , @ricardosalveti |
|
After adding qcbor to the Command: |
|
I think Ricardo meant to drop QCBOR from this layer and use qcbor dynamically from meta-oe. |
Hi @vkraleti , @vksharma-oss , if I use qcbor dynamically from meta-oe, I am unable to build security components standalone with oe-core without relying on other layers such as meta-oe. Hi @lumag , if I use qcbor dynamically from meta-oe, could you share the compilation command? The README only mentions the |
This is inline with expectations. Security features are typically tied to distributions and are not default offerings in BSP layers. Similarly for this case, you need to :
|
Meta-oe is added when building with meta-qcom-distro, so you will have to use |
How can we include minkipc and qcomtee in the image for validation purposes? |
Hi @vkraleti @lumag , why we should move the minkipc and qcbor in dynamic-layer? This two repo is owned by qcom, can we create the |
Ask is to drop qcbor recipe from meta-qcom and use the one in meta-oe. Since qcomtee and minkipc recipes are dependent on qcbor, which would be available only when meta-oe is parsed, enabling them by default will cause build issues. Keeping them under "dynamic-layers/openembedded-layer/recipes-security" allows to skip these when meta-oe is not present. |
Have we confirmed the build target? I'm wondering why the device isn't booting up after flashing the build with no modifications. |
Yes, multimedia-image built with 'qcom-distro' is booting fine on rb3gen2-core-kit. This combination is in CI as well. |
|
Thank you for your attention and for reviewing the code. I will close this PR and use the qcbor library from meta-oe. |
Add a recipe to build qcbor, library to work with security functionality on Qualcomm platforms.
The libqcbor, which are only available through meta-oe, is depended by Qcom-teec and MinkIPC. Thus add libqcbor recipe to the corresponding dynamic-layers subdir.
The qcbor library is used for decoding and encoding data structures.