lkml.org 
[lkml]   [2022]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver
    From
    Hi Hans,

    On 4/4/22 3:07 AM, Hans de Goede wrote:
    >> +static int __init tdx_attest_init(void)
    >> +{
    >> + dma_addr_t handle;
    >> + long ret = 0;
    >> +
    >> + mutex_lock(&attestation_lock);
    >> +
    >> + ret = misc_register(&tdx_attest_device);
    >> + if (ret) {
    >> + pr_err("misc device registration failed\n");
    >> + mutex_unlock(&attestation_lock);
    >> + return ret;
    >> + }
    > Why not do this as the last thing of the probe?

    We need misc device reference in dma_alloc_coherent() and
    dma_set_coherent_mask() calls. This is the reason for keeping
    misc_register() at the beginining of the init function.

    >
    > That will avoid the need to unregister this again in all
    > the error-exit paths and also fixes a possible deadlock.
    >

    Agree. But, unless we create another device locally, I don't
    think we can avoid this. Do you prefer this approach?

    > Right now you possibly have:
    >
    > 1. probe() locks attestation_lock
    > 2. probe() registers misc-device
    > 3. userspace calls tdx_attest_ioctl
    > 4. tdx_attest_ioctl blocks waiting for attestastion_lock
    > 5. Something goes wrong in probe, probe calls
    > misc_deregister()
    > 6. misc_deregister waits for the ioctl to finish
    > 7. deadlock
    >
    > I'm not sure about 6, but if 6 does not happen then
    > instead we now have tdx_attest_ioctl running
    > after the misc_deregister, with tdquote_data and
    > tdreport_data as NULL, or pointing to free-ed memory
    > leading to various crash scenarios.

    Makes sense. But as I have mentioned above, we have reason
    for keeping the misc_register() at the begining of the
    init function.

    One way to avoid this deadlock is to use global initalization
    check.

    --- a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
    +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
    @@ -48,6 +48,8 @@ static void *tdreport_data;
    /* DMA handle used to allocate and free tdquote DMA buffer */
    dma_addr_t tdquote_dma_handle;

    +static bool device_initialized;
    +
    static void attestation_callback_handler(void)
    {
    complete(&attestation_done);
    @@ -60,6 +62,9 @@ static long tdx_attest_ioctl(struct file *file,
    unsigned int cmd,
    struct tdx_gen_quote tdquote_req;
    long ret = 0;

    + if (!device_initialized)
    + return -ENODEV;
    +
    mutex_lock(&attestation_lock);

    switch (cmd) {
    @@ -191,6 +196,8 @@ static int __init tdx_attest_init(void)

    mutex_unlock(&attestation_lock);

    + device_initialized = true;
    +
    pr_debug("module initialization success\n");

    return 0;
    Please let me know your comment on above solution.

    >
    > TL;DR: you must always delay registering any
    > interfaces for userspace until your code is
    > ready to deal with userspace calls.
    >
    > Regards,
    >
    > Hans
    >
    > p.s.
    >
    > As I mentioned with v1:
    >
    >

    --
    Sathyanarayanan Kuppuswamy
    Linux Kernel Developer

    \
     
     \ /
      Last update: 2022-04-05 00:16    [W:2.257 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site