Received: by 2002:ac0:a5b6:0:0:0:0:0 with SMTP id m51-v6csp4708274imm; Wed, 30 May 2018 10:22:37 -0700 (PDT) X-Google-Smtp-Source: ADUXVKLB1eU3s8R7q6LS2IXS2GaLVTb4Kgau2oGbfSB6nLkqy0fwkLuIMLZTIMc+LQ2oIDrnqPZV X-Received: by 2002:a62:581:: with SMTP id 123-v6mr3636007pff.38.1527700957273; Wed, 30 May 2018 10:22:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1527700957; cv=none; d=google.com; s=arc-20160816; b=NA9mU3ueFGZn1BTlwSimYkrVv4fn+zu1ly+L5bFSqEilEXYDhNQcb6qSYz4Brc924C eI7EJfAJbaNumM+rEEtRclv38Ub1+8esF66rM1c9uZYZx7oTrPZ7Zq2CqxPaFjEYMh8F JC1Nn7JjwhWPI9yKwW+8LNol+Lz6erol4N5stIoaTYgxphjqYcLrJdUcN7zwKzKa6t8F Q/KO0e542N0gP87J/7zaOh7gYg0arBKVmvww88QIR2zRjaQbmp5lRa225UPmwY/Q5ufO GyCtfgevfbcmoB6Zh9b+vTRvNMgV5JO/tqN6OpD1D6eKaBKk0sWqUOVtWOIUKQUVDhMT /+eA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature :arc-authentication-results; bh=6bL0EmqIs0RlfLuh7ENbdn+yLzaS+zh1bSFZHiQrDp8=; b=nz4R1MOQ4lFMkeFTBcmT1EdQzKiocc9bEYTpYC3MLoPkcEQvoAyZtElFo1atwR2D7c g42Q1YGNaY1d4Jx0UDYlkbkZrLoMT/QxMoA3amylPqIFtoE49R2ge0548Aeh/HImzq4C 8YHE2yWqQfs3rG/oL0H5vJyLY4iYqPzbK3o44MRHJNewEdRwFZcOgRpMRj3rD4DWEBcS fLspiPtQ0I31FFvVqDdIvUZrzcPt0KiH58KEjBFHuvfKm6s4U8wBVtgZPew/6ySwmP39 6TF6TD/0QJaywYYLM55WiC895bqNMwx7vPafvmuG9Zdx+bkihtJpL2NSHKSElVU5F8W8 hpYw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=NFj3mak6; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id ay3-v6si34930524plb.361.2018.05.30.10.22.23; Wed, 30 May 2018 10:22:37 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=NFj3mak6; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753604AbeE3RVw (ORCPT + 99 others); Wed, 30 May 2018 13:21:52 -0400 Received: from mail-ot0-f195.google.com ([74.125.82.195]:46029 "EHLO mail-ot0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752980AbeE3RVu (ORCPT ); Wed, 30 May 2018 13:21:50 -0400 Received: by mail-ot0-f195.google.com with SMTP id 15-v6so22013940otn.12 for ; Wed, 30 May 2018 10:21:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=6bL0EmqIs0RlfLuh7ENbdn+yLzaS+zh1bSFZHiQrDp8=; b=NFj3mak62mTsjZJTrBGTNktO4T3lWVuHvNfRYWJWS58LpqnJGgGX7/C2bHgZ6EGJpp f8t6l+kyQKQnhYijfJ+bBXEFiDnVyeKL14sL5kQJ+tvZ9eoe5a7BZdkyWZ3b/InjI9AI 77tfaxDLcAdZjtua9uiXBmYW4BVJDC4Assf+s= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6bL0EmqIs0RlfLuh7ENbdn+yLzaS+zh1bSFZHiQrDp8=; b=O7wcrCh6oraU/qoVW7W3i1LHZXWmkfv3T9VtjZUjw6WK5r033VvK8KrBOqBSmRfqfL L64kuqya8rUMZ23NPt+K8AP25uPlEErmd1lI0J9yXD/TQ7lGhrbQEQdMhC5f0tfEH5xD kI0XLKN9u4wJwb2yUvjwoH8TnG0kOVaKhongxLeN+ebWUccdUF4QXveLw2qbwO7AzFA9 ve54I7HwL3roB/X1645WGkXlseCYPSR5lUihJ82zL/byqSC+YpfH2cw0rY50u2o/sPrd gO4M3v++0RNKyXmr3aoL6fvrSDyVHVli5rOpSKJnI7JCa7S9Cngjvy3mGFoYdX+JjpgM Yu1A== X-Gm-Message-State: ALKqPwek5cgoPkY4LLRntB0+vGRV7ztUij34vAOMqY8FYndicVwmiHY5 sTUU9ruuXmkVrdFtOCNdNaCEsAxU+Fc= X-Received: by 2002:a9d:2f91:: with SMTP id r17-v6mr2517466otb.356.1527700910096; Wed, 30 May 2018 10:21:50 -0700 (PDT) Received: from mail-oi0-f52.google.com (mail-oi0-f52.google.com. [209.85.218.52]) by smtp.gmail.com with ESMTPSA id w88-v6sm5673871ota.48.2018.05.30.10.21.47 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 30 May 2018 10:21:48 -0700 (PDT) Received: by mail-oi0-f52.google.com with SMTP id t27-v6so17042046oij.9 for ; Wed, 30 May 2018 10:21:47 -0700 (PDT) X-Received: by 2002:aca:3841:: with SMTP id f62-v6mr1935177oia.135.1527700907437; Wed, 30 May 2018 10:21:47 -0700 (PDT) MIME-Version: 1.0 References: <20180529181740.195362-1-evgreen@chromium.org> <20180529181740.195362-6-evgreen@chromium.org> In-Reply-To: <20180529181740.195362-6-evgreen@chromium.org> From: Evan Green Date: Wed, 30 May 2018 10:21:10 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 5/7] scsi: ufs: Refactor descriptor read for write To: vinholikatti@gmail.com, jejb@linux.vnet.ibm.com, martin.petersen@oracle.com, stanislav.nijnikov@wdc.com, linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org Cc: gwendal@chromium.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Reviewing my own patch... On Tue, May 29, 2018 at 11:18 AM Evan Green wrote: > This change refactors the ufshcd_read_desc_param function into > one that can both read and write descriptors. Most of the low-level > plumbing for writing to descriptors was already there, this change > simply enables that code path, and manages the incoming data buffer > appropriately. > Signed-off-by: Evan Green > --- > drivers/scsi/ufs/ufs-sysfs.c | 4 +- > drivers/scsi/ufs/ufshcd.c | 89 ++++++++++++++++++++++++++++++++------------ > drivers/scsi/ufs/ufshcd.h | 13 ++++--- > 3 files changed, 74 insertions(+), 32 deletions(-) ... > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 00e79057f870..6a5939fb5da3 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -3004,22 +3004,24 @@ int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, > EXPORT_SYMBOL(ufshcd_map_desc_id_to_length); > /** > - * ufshcd_read_desc_param - read the specified descriptor parameter > + * ufshcd_rw_desc_param - read or write the specified descriptor parameter > * @hba: Pointer to adapter instance > + * @opcode: indicates whether to read or write > * @desc_id: descriptor idn value > * @desc_index: descriptor index > - * @param_offset: offset of the parameter to read > - * @param_read_buf: pointer to buffer where parameter would be read > - * @param_size: sizeof(param_read_buf) > + * @param_offset: offset of the parameter to read or write > + * @param_buf: pointer to buffer to be read or written > + * @param_size: sizeof(param_buf) > * > * Return 0 in case of success, non-zero otherwise > */ > -int ufshcd_read_desc_param(struct ufs_hba *hba, > - enum desc_idn desc_id, > - int desc_index, > - u8 param_offset, > - u8 *param_read_buf, > - u8 param_size) > +int ufshcd_rw_desc_param(struct ufs_hba *hba, > + enum query_opcode opcode, > + enum desc_idn desc_id, > + int desc_index, > + u8 param_offset, > + u8 *param_buf, > + u8 param_size) > { > int ret; > u8 *desc_buf; > @@ -3042,24 +3044,57 @@ int ufshcd_read_desc_param(struct ufs_hba *hba, > return ret; > } > + if (param_offset > buff_len) > + return 0; > + > /* Check whether we need temp memory */ > if (param_offset != 0 || param_size < buff_len) { > - desc_buf = kmalloc(buff_len, GFP_KERNEL); > + desc_buf = kzalloc(buff_len, GFP_KERNEL); > if (!desc_buf) > return -ENOMEM; > + > + /* If it's a write, first read the complete descriptor, then > + * copy in the parts being changed. > + */ > + if (opcode == UPIU_QUERY_OPCODE_WRITE_DESC) { > + if ((int)param_offset + (int)param_size > buff_len) { > + ret = -EINVAL; > + goto out; > + } > + > + ret = ufshcd_query_descriptor_retry(hba, > + UPIU_QUERY_OPCODE_READ_DESC, > + desc_id, desc_index, 0, > + desc_buf, &buff_len); > + > + if (ret) { > + dev_err(hba->dev, > + "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d", > + __func__, desc_id, desc_index, > + param_offset, ret); > + > + goto out; > + } > + > + memcpy(desc_buf + param_offset, param_buf, param_size); > + } > + > } else { > - desc_buf = param_read_buf; > + desc_buf = param_buf; > is_kmalloc = false; > } > - /* Request for full descriptor */ > - ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC, > + /* Read or write the entire descriptor. */ > + ret = ufshcd_query_descriptor_retry(hba, opcode, > desc_id, desc_index, 0, > desc_buf, &buff_len); > if (ret) { > - dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d", > - __func__, desc_id, desc_index, param_offset, ret); > + dev_err(hba->dev, "%s: Failed %s descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d", > + __func__, > + opcode == UPIU_QUERY_OPCODE_READ_DESC ? > + "reading" : "writing", > + desc_id, desc_index, param_offset, ret); > goto out; > } > @@ -3071,12 +3106,16 @@ int ufshcd_read_desc_param(struct ufs_hba *hba, > goto out; > } > - /* Check wherher we will not copy more data, than available */ > - if (is_kmalloc && param_size > buff_len) > - param_size = buff_len; > + /* Copy data to the output. The offset is already validated to be > + * within the buffer. > + */ > + if (is_kmalloc && (opcode == UPIU_QUERY_OPCODE_READ_DESC)) { > + if ((int)param_offset + (int)param_size > buff_len) > + param_size = buff_len - param_offset; > + > + memcpy(param_buf, &desc_buf[param_offset], param_size); > + } Previously this function didn't handle param_offset being non-zero well at all, as it would allow you to read off the end of the descriptor. My hunk here, as well as the "if (param_offset > buff_len) return 0;" above prevent reading off the end of the kmalloced buffer. But it will return success while leaving the caller's buffer uninitialized, which I think is asking for trouble. I think I should fail if the starting offset is greater than the size. If the requested read starts at a valid index but ends beyond the end, I think I should clear the remaining buffer. ufshcd_read_string_desc, for example, always asks for the max, and would need this bit of leniency. -Evan