From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9C961C433F5 for ; Fri, 1 Oct 2021 19:08:50 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9274F6137A for ; Fri, 1 Oct 2021 19:08:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 9274F6137A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id F3D1180F9D; Fri, 1 Oct 2021 21:08:46 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="UklnbOTs"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 84DAF81196; Fri, 1 Oct 2021 21:08:45 +0200 (CEST) Received: from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com [IPv6:2a00:1450:4864:20::42f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 3C45B80F3B for ; Fri, 1 Oct 2021 21:08:42 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-wr1-x42f.google.com with SMTP id r10so501256wra.12 for ; Fri, 01 Oct 2021 12:08:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=HboLHpL9dZq6DO9tS7/infVD8Rlon1gRmnPYtV21+qk=; b=UklnbOTsBtW2vmODwe7sffjbvgLK0tV7JJZbmMQM6s0BxkYxICg/4oG0NFhrMTJ8S2 Ju5pFbGAOR3NfTeZo7xspyBtb5tEQoQ0z8UHrVdQjr23fBt7ROSg2faPhU8KXjj5BYlY uKct5auuJ0cD01R6IbqxkqTrET1ztJl/m+kRyn47fw8iA1vGa3lkG3NFpFlZ4jsV8/rT NV+iAi4TJzJwdcR+sHG05miSbO/Cdwm8iF0p2eWlKJYDcp5kHZRp2CXLla81QpMnimYv lbYRkTeuDyBeXH3n0hrieL0RRrUpab/79wVQn178BtOwmJm8Vbpon3XHVBhxaKM7Gc2V EfXQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=HboLHpL9dZq6DO9tS7/infVD8Rlon1gRmnPYtV21+qk=; b=FKT7ng0TMuZhUzRlurQSyV6SHVK1zw+Sd/uqnNUG6u2DGE+Q1vCYAPDqW1oUtGeoAP 1TslwNkFRPUH4aHjdOkZSp4TGhPC4jSc49SeKjVUoZsIvwwfPna0DFXE8t0E/hVsX0T3 soa45Z4L52O7dalYfEZoP+BKuOjKkN2zXif5/1UE30uwx5qKYbLGtMwf3AoRe66La+Zx c1eDNMr7J8rr7GqoND6P5dWkkXyDX9IOHPgDzvVn0jc10MkU1BeU+AWS7kqb+C6kdKtS VAGtOpN55oavMixbiJiQWcypmrw37Rs1Q2el1VSYlKwwFrrk2hiaZOJJ/qJU/tXFNHh9 famg== X-Gm-Message-State: AOAM530XRm2Uwzs/KBvE5cLWjAH2iul/UyJUkCpCfM3EoO8VXClxW+x+ qlSOshFYCYhwWMr5RhuZDMZVZA== X-Google-Smtp-Source: ABdhPJwWqGW4Cmjko1fU1oVYY8o6IECChWlbfbOuER+9S46WGuChzt7vq16XS65DysCKeUdOEhS/LA== X-Received: by 2002:a5d:64a7:: with SMTP id m7mr14683690wrp.171.1633115321778; Fri, 01 Oct 2021 12:08:41 -0700 (PDT) Received: from apalos.home (ppp-94-66-220-209.home.otenet.gr. [94.66.220.209]) by smtp.gmail.com with ESMTPSA id d2sm6493382wrc.32.2021.10.01.12.08.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 01 Oct 2021 12:08:41 -0700 (PDT) Date: Fri, 1 Oct 2021 22:08:38 +0300 From: Ilias Apalodimas To: Heinrich Schuchardt Cc: U-Boot Mailing List , AKASHI Takahiro , Sughosh Ganu , Masahisa Kojima , Alexander Graf Subject: Re: [PATCH 3/4] efi_loader: simplify efi_sigstore_parse_sigdb() Message-ID: References: <20210911072832.16991-1-xypron.glpk@gmx.de> <20210911072832.16991-4-xypron.glpk@gmx.de> <7812832d-5cc3-da7e-e274-030d696b5537@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7812832d-5cc3-da7e-e274-030d696b5537@gmx.de> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean Hi Heinrich, On Fri, Oct 01, 2021 at 06:42:14PM +0200, Heinrich Schuchardt wrote: > > > On 9/12/21 21:23, Ilias Apalodimas wrote: > > Hi Heinrich > > > > [...] > > > > > - if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) { > > > > > - vendor = &efi_global_variable_guid; > > > > > - } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) { > > > > > - vendor = &efi_guid_image_security_database; > > > > > - } else { > > > > > + vendor = efi_auth_var_get_guid(name); > > > > > + if (!vendor) { > > > > > EFI_PRINT("unknown signature database, %ls\n", name); > > > > > return NULL; > > > > > } > > > > > > > > efi_auth_var_get_guid() will return &efi_global_variable_guid if the > > > > GUID for the variable name isn't found. > > > > > > Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID. > > > I want to reuse the same function there in future. > > > > > > Best regards > > > > Then I guess the check can go away ? > > Yes > > > > > > > > > Heinrich > > > > > > > > > > > > > > > > > - /* retrieve variable data */ > > > > > - db_size = 0; > > > > > - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL)); > > > > > - if (ret == EFI_NOT_FOUND) { > > > > > - EFI_PRINT("variable, %ls, not found\n", name); > > > > > - sigstore = calloc(sizeof(*sigstore), 1); > > > > > - return sigstore; > > > > > - } else if (ret != EFI_BUFFER_TOO_SMALL) { > > > > > - EFI_PRINT("Getting variable, %ls, failed\n", name); > > > > > - return NULL; > > > > > - } > > > > > - > > > > > - db = malloc(db_size); > > > > > + db = efi_get_var(name, vendor, &db_size); > > > > > if (!db) { > > > > > - EFI_PRINT("Out of memory\n"); > > > > > - return NULL; > > > > > - } > > > > > - > > > > > - ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db)); > > > > > - if (ret != EFI_SUCCESS) { > > > > > - EFI_PRINT("Getting variable, %ls, failed\n", name); > > > > > - free(db); > > > > > - return NULL; > > > > > + EFI_PRINT("variable, %ls, not found\n", name); > > > > > + return calloc(sizeof(struct efi_signature_store), 1); > > > > Why? From the patch alone it's not clear why you want to allocate > > memory here instead of returning NULL. > > This is existing code. See the same lines deleted above. If I read the code correctly, we are trying to be smart about the buffer outcome. Check for example efi_image_unsigned_authenticate(). By returning an empty buffer the 'dbx' check will succeed but the 'db' check a few lines after will fail. But this is pointless imho... Why don't we just have efi_status_t efi_signature_store efi_sigstore_parse_sigdb(u16 *name, struct efi_signature_store *store) We can the control the EFI return value in efi_sigstore_parse_sigdb() and any callers would just have to look at the result, instead of getting a memory that contains empty data and try to reason about it. IOW you can check for EFI_NOT_FOUND in both cases on the caller function. If you are working with 'dbx' then that's fine and you can continue. If you are working with 'db' you need to fail the authentication. This imho is much more readable. Regards /Ilias