From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerry Hoemann Subject: Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls Date: Mon, 11 Jan 2016 18:32:59 -0700 Message-ID: <20160112013259.GC79247@tevye.fc.hp.com> References: Reply-To: Jerry.Hoemann@hpe.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Dan Williams Cc: Ross Zwisler , "Rafael J. Wysocki" , Len Brown , "Elliott, Robert (Persistent Memory)" , jmoyer , Dmitry Krivenok , Linda Knippers , Robert Moore , Lv Zheng , Rafael J Wysocki , "linux-nvdimm@lists.01.org" , Linux ACPI , "linux-kernel@vger.kernel.org" List-Id: linux-acpi@vger.kernel.org On Sun, Jan 10, 2016 at 04:03:18PM -0800, Dan Williams wrote: > On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams wrote: > > On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann wrote: > [..] > >> Jerry Hoemann (6): > >> ACPI / util: Fix acpi_evaluate_dsm() argument type > >> nvdimm: Clean-up access mode check. > >> nvdimm: Add wrapper for IOCTL pass thru > >> nvdimm: Fix security issue with DSM IOCTL. > >> nvdimm: Increase max envelope size for IOCTL > >> nvdimm: Add IOCTL pass thru functions > > > > These look good to me. > > > > I'll tag "nvdimm: Fix security issue with DSM IOCTL." for -stable. > > > > Thanks Jerry! > > I went to go write a test / support in ndctl for these and noticed a > few things I want to address before merging. > > 1/ Advertise 'call_dsm' as a supported command alongside the others. In sysfs? okay that makes sense. > > 2/ Disallow potentially invalid calls to reach firmware. At a minimum > the kernel needs to know the uuid in advance for any dsm it wants to > send. I.e. check the 'dsm_fun_idx' against the dsm_mask. This is > also important for making sure the kernel can manage exclusive access > to the configuration data area if present > (ND_CMD_{GET|SET}_CONFIG_DATA). Technically, the kernel doesn't need to know the uuid in advance as that is part of the bundle passed into the passthru. Are you concerned about firmware mis-behaving when presented with a (UUID, Function_Index) that is not supported? (and really we should add Revision ID to that tuple.) In a prior version of the patch not sent upstream, I did "discover" the uuid and set up the dsm_mask. However, this created a need to modify kernel each time uuid changes. Also, i don't think this is necessary as FW should be gracefully validating its input arguments. By not setting up/using dsm_mask in pass thru case, this can be tested. I don't understand the exclusive access concern w/ config data. Could you please elaborate? > > 3/ This is minor, but it follows from 1/ that there may be some nvdimm > bus implementations that do not implement 'call_dsm' support. > 'nfit_test' is currently one of those buses and we need to check for > that explicitly in nd_ioctl. > > I have some patches in progress to address these. > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- ----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise ----------------------------------------------------------------------------- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761975AbcALBdG (ORCPT ); Mon, 11 Jan 2016 20:33:06 -0500 Received: from g2t1383g.austin.hp.com ([15.217.136.92]:38363 "EHLO g2t1383g.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761667AbcALBdE (ORCPT ); Mon, 11 Jan 2016 20:33:04 -0500 Date: Mon, 11 Jan 2016 18:32:59 -0700 From: Jerry Hoemann To: Dan Williams Cc: Ross Zwisler , "Rafael J. Wysocki" , Len Brown , "Elliott, Robert (Persistent Memory)" , jmoyer , Dmitry Krivenok , Linda Knippers , Robert Moore , Lv Zheng , Rafael J Wysocki , "linux-nvdimm@lists.01.org" , Linux ACPI , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls Message-ID: <20160112013259.GC79247@tevye.fc.hp.com> Reply-To: Jerry.Hoemann@hpe.com References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Jan 10, 2016 at 04:03:18PM -0800, Dan Williams wrote: > On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams wrote: > > On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann wrote: > [..] > >> Jerry Hoemann (6): > >> ACPI / util: Fix acpi_evaluate_dsm() argument type > >> nvdimm: Clean-up access mode check. > >> nvdimm: Add wrapper for IOCTL pass thru > >> nvdimm: Fix security issue with DSM IOCTL. > >> nvdimm: Increase max envelope size for IOCTL > >> nvdimm: Add IOCTL pass thru functions > > > > These look good to me. > > > > I'll tag "nvdimm: Fix security issue with DSM IOCTL." for -stable. > > > > Thanks Jerry! > > I went to go write a test / support in ndctl for these and noticed a > few things I want to address before merging. > > 1/ Advertise 'call_dsm' as a supported command alongside the others. In sysfs? okay that makes sense. > > 2/ Disallow potentially invalid calls to reach firmware. At a minimum > the kernel needs to know the uuid in advance for any dsm it wants to > send. I.e. check the 'dsm_fun_idx' against the dsm_mask. This is > also important for making sure the kernel can manage exclusive access > to the configuration data area if present > (ND_CMD_{GET|SET}_CONFIG_DATA). Technically, the kernel doesn't need to know the uuid in advance as that is part of the bundle passed into the passthru. Are you concerned about firmware mis-behaving when presented with a (UUID, Function_Index) that is not supported? (and really we should add Revision ID to that tuple.) In a prior version of the patch not sent upstream, I did "discover" the uuid and set up the dsm_mask. However, this created a need to modify kernel each time uuid changes. Also, i don't think this is necessary as FW should be gracefully validating its input arguments. By not setting up/using dsm_mask in pass thru case, this can be tested. I don't understand the exclusive access concern w/ config data. Could you please elaborate? > > 3/ This is minor, but it follows from 1/ that there may be some nvdimm > bus implementations that do not implement 'call_dsm' support. > 'nfit_test' is currently one of those buses and we need to check for > that explicitly in nd_ioctl. > > I have some patches in progress to address these. > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- ----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise -----------------------------------------------------------------------------