Received: by 2002:a05:6358:11c7:b0:104:8066:f915 with SMTP id i7csp4863316rwl; Mon, 3 Apr 2023 10:37:41 -0700 (PDT) X-Google-Smtp-Source: AKy350ZmQvoIPFLxq3lcudKmebz2TNwBBrkYnZADKz/ysAvBrK09Sy6Yu3GmY2HDOHMj2jht605P X-Received: by 2002:a17:90b:4a50:b0:23d:21c9:193 with SMTP id lb16-20020a17090b4a5000b0023d21c90193mr40416940pjb.2.1680543461321; Mon, 03 Apr 2023 10:37:41 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680543461; cv=none; d=google.com; s=arc-20160816; b=uzdushJ8eTR601HlOhFgcCzzqtCjWTe9tEKo+4fz1l0XFyYeVqm5Vup1O6Reon8Zy/ A1KUTKl+4AFMo+9i8PoezhJCHJ2QLLjrwciS1najOOxu6QTAyjpphex8m+U/GIMatjRT 7E7g2xnWTdKQ7O7peX81jH+8ZmAGLmVzYgKKRrmHVZT8ytcIgxnHdz4aDP44pzVgLH3/ ZWCcFo6a4stcskSm5BrY6TmR3wKvf41oXF4nTOFoHiX9hH4e71+m40TkRlp64spiswPu lAm02WIXttr2HTJe91CZ9dLKSI84tytwy3RWIFVVEdvldw5Z7QUXdL/YVifyziBO+BNL 4vjg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:dkim-signature:date; bh=EM421G/32mP/P8oBgjGCXyAxmCmhER3BhKF8XLmQubg=; b=hGaQmAqKhMbfKtgPtDj5UTAkkRoV1qPqt2QLUFq3BDJxtxEmoq5KRwO3vFB+AjxJeM fJSx6lV0AUineAVyvMEjFUUKfOXifpPLUM72PSkgyf30Fp615dGX2ptGz6GK7tC3fKrI MorSbnJhfxerSDJurkXOGAeN9tOHewN2+X/JzX6h8eLwaH7H9XFcbnQmiiRoAUAz85pA 84NXdvfYtYum4q3NpErdNP1lT3cncVBNVuHq6j3JNGsucv4AqXBgy3E2jx4kiVNLsNX5 lvwQOi4Js3j41QUPitX4UKea5n4MuTYHIXjQ2htHqA3QtqlqJe7jV2U1nU+lylc9ACoh 6+/A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@t-8ch.de header.s=mail header.b=NFNxEvXA; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id o12-20020a17090ac70c00b002335ea8726dsi8374826pjt.88.2023.04.03.10.37.25; Mon, 03 Apr 2023 10:37:41 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@t-8ch.de header.s=mail header.b=NFNxEvXA; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233151AbjDCRaO (ORCPT + 99 others); Mon, 3 Apr 2023 13:30:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35560 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231245AbjDCRaN (ORCPT ); Mon, 3 Apr 2023 13:30:13 -0400 Received: from todd.t-8ch.de (todd.t-8ch.de [159.69.126.157]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 11AC9B4; Mon, 3 Apr 2023 10:30:10 -0700 (PDT) Date: Mon, 3 Apr 2023 17:30:06 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=t-8ch.de; s=mail; t=1680543008; bh=B0pj9cDi6NvqvG3KIfUUAJRoeMd/oY5yI/vrKxSofLI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=NFNxEvXAZAUlk5bsrNr4xxkAI03D2xxJOfZqcu6zy1KcCSJAJPm0piCHg6X//ZvzD wQz0oYanNLycG6sr72sa0xmChGbMwqlEDsUwuljmOhkZ6PZQcdhKkTdLpLkaMzhdBD YaiptmxWjJQf7wW4rHPxPGBK+OI3hD5QCGJlNve4= From: Thomas =?utf-8?Q?Wei=C3=9Fschuh?= To: Jorge Lopez Cc: hdegoede@redhat.com, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 4/4] Introduction of HP-BIOSCFG driver [4] Message-ID: References: <20230309201022.9502-1-jorge.lopez2@hp.com> <20230309201022.9502-5-jorge.lopez2@hp.com> <6da33dcc-0526-4398-bf35-655b64d07e20@t-8ch.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jorge, On 2023-04-03 11:33:20-0500, Jorge Lopez wrote: > Hi Thomas, > > Please see my comments below. > > On Sat, Apr 1, 2023 at 6:58 AM Thomas Weißschuh wrote: > > On 2023-03-09 14:10:22-0600, Jorge Lopez wrote: > [..] > > > HP BIOS Configuration driver purpose is to provide a driver supporting > > > the latest sysfs class firmware attributes framework allowing the user > > > to change BIOS settings and security solutions on HP Inc.’s commercial > > > notebooks. > > > > Here it says "notebooks", below "PC's". Does it also support > > non-notebook machines? > > The initial release of the driver will be supported for business notebooks. > Although the driver is not targeted for non-notebooks machines, the > driver was tested on non-notebooks in the event a decision is made to > targets them If it is not intended to support both, maybe the documentation could consistently use "notebook". > > > + "sure-start"-type specific properties: > > > + > > > + audit_log_entries: > > > + A read-only file that returns the events in the log. > > > + > > > + Audit log entry format > > > + > > > + Byte 0-15: Requested Audit Log entry (Each Audit log is 16 bytes) > > > + Byte 16-127: Unused > > > + > > > + audit_log_entry_count: > > > + A read-only file that returns the number of existing audit log events available to be read. > > > + > > > + [No of entries],[log entry size],[Max number of entries supported] > > > > sysfs is based on the idea of "one-value-per-file". > > The two properties above violate this idea. > > Maybe a different interface is needed. > > > > Both properties report a single string separated by semicolon. This > is not different from listing all elements in a single string > separated by semicolon. The documentation does not mention semicolons. The nice thing about descoping functionality is that we don't need to worry about their details now. Instead it can be added later without haste as the core functionality can already be used by the users. > > Are these properties very important for the first version of this > > driver? If not I would propose to drop them for now and resubmit them > > as separate patches after the main driver has been merged. > > > We want the initial driver to have all predefined properties available > first. There are plans to add future properties and features which > will be submitted as patches. With "properties" do you mean the bios settings? I agree that all these are good for the initial driver. But the audit log, detailed error codes, etc... do not seem integral for the functioning of the driver or for users. > > > + HP specific class extensions > > > + -------------------------------- > > > + > > > +What: /sys/class/firmware-attributes/*/authentication/SPM/kek > > > +Date: March 29 > > > +KernelVersion: 5.18 > > > +Contact: "Jorge Lopez" > > > +Description: 'kek' is a write-only file that can be used to configure the > > > + RSA public key that will be used by the BIOS to verify > > > + signatures when setting the signing key. When written, > > > + the bytes should correspond to the KEK certificate > > > + (x509 .DER format containing an OU). The size of the > > > + certificate must be less than or equal to 4095 bytes. > > > + > > > + > > > +What: /sys/class/firmware-attributes/*/authentication/SPM/sk > > > +Date: March 29 > > > +KernelVersion: 5.18 > > > +Contact: "Jorge Lopez" > > > +Description: 'sk' is a write-only file that can be used to configure the RSA > > > + public key that will be used by the BIOS to verify signatures > > > + when configuring BIOS settings and security features. When > > > + written, the bytes should correspond to the modulus of the > > > + public key. The exponent is assumed to be 0x10001. > > > > The names of the files 'SPM', 'kek' and 'sk' are cryptic. > > SPM - Secure Platform Manager > kek - Key-Encryption-Key (KEK) > sk - Signature Key (SK) > > Those abbreviations were used because they are industry standard and > reduce the size of the commands. Any suggestions? Maybe mention the long names once in the documentation "Description". > > > + > > > + > > > +What: /sys/class/firmware-attributes/*/authentication/SPM/status > > > +Date: March 29 > > > +KernelVersion: 5.18 > > > +Contact: "Jorge Lopez" > > > +Description: 'status' is a read-only file that returns ASCII text reporting > > > + the status information. > > > + > > > + State: Not Provisioned / Provisioned / Provisioning in progress > > > + Version: Major. Minor > > > + Feature Bit Mask: <16-bit unsigned number display in hex> > > > + SPM Counter: <16-bit unsigned number display in base 10> > > > + Signing Key Public Key Modulus (base64): > > > + KEK Public Key Modulus (base64): > > > > This also violates 'one-value-per-file'. > > Can it be split into different files? > > I will split the information in multiple files. > > > This would also remove the need for the statusbin file. > > > Status bin is used by GUI applications where data is managed > accordingly instead of individual lines. Can the GUI applications not use the split files? > > For the values: > > > > Status: I think symbolic names are better for sysfs: > > not_provisioned, provisioned, etc. > > Feature Bit Mask: Use names. > > Keys: It would be nicer if these could be shown directly in the files > > that can be used to configure them. > > > > As before, what is really needed and what can be added later? > > Status is needed when the user enables Secure Platform Manager in BIOS > and KEK and/or SK are configured. Ok. > > > > > + > > > + > > > +What: /sys/class/firmware-attributes/*/authentication/SPM/statusbin > > > +Date: March 29 > > > +KernelVersion: 5.18 > > > +Contact: "Jorge Lopez" > > > +Description: 'statusbin' is a read-only file that returns identical status > > > + information reported by 'status' file in binary format. > > > > How does this binary format work? > > Yes. Status bin is used by GUI applications where data is managed > accordingly instead of individual lines But this format is not documented here at all. So how can we determine if the implementation is correct? > > > + > > > + > > > +What: /sys/class/firmware-attributes/*/attributes/last_error > > > +Date: March 29 > > > +KernelVersion: 5.18 > > > +Contact: "Jorge Lopez" > > > +Description: 'last_error' is a read-only file that returns WMI error number > > > + and message reported by last WMI command. > > > > Does this provide much value? > > Or could this error just be logged via pr_warn_ratelimited()? > > It is specially needed to determine if WMI calls reported an error. > This property is similar to the one provided by both Dell and Lenovo > drivers I don't see similar functionality for the other drivers. Instead they seem to just return the error codes from the attribute callbacks. This may be useful but it does not seem *necessary* for the first version. Feel free to only submit the patch with the documentation for the next revision. Then we can nail down the interface and initial functionality and you don't always have to adapt the code to the changing interface. Thomas