From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::42a; helo=mail-wr1-x42a.google.com; envelope-from=roman.bacik@broadcom.com; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B7250203B99CE for ; Mon, 9 Jul 2018 17:30:43 -0700 (PDT) Received: by mail-wr1-x42a.google.com with SMTP id s11-v6so12726819wra.13 for ; Mon, 09 Jul 2018 17:30:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=9NNZufkVtfW1zudUByMoWi9gjVBxIvRBGKaB/NAmWWQ=; b=POHQyd7PzbnXn8nk/LytjG22M9ji8EndBQ+4ssdABP1B4Se1tFKaiOge1NWXf0H3Cg 3rBXwoNLLKs9a9K+tmOT4SRYBEuSXoIYACtjr9549NuUDG+577HMRWUPUsvgW5uftzaH YGd6ovN424PJ223EEnSG8TnZiEGT9rGrSPYAg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=9NNZufkVtfW1zudUByMoWi9gjVBxIvRBGKaB/NAmWWQ=; b=lSGABuskeAUanq1wDspKPiGc423huAFdG3iUOP/0x1s26xIcHRq0INmASxvUbeLahc it7fdtC3rKeQDPyu8Y6AAulHmzYkPs+d4P7r6Q3FvMy33ETQ/Gf3CDdcecxoZ2dpQlRR zFU1EFjrCq35nRMs2bOkJ9aIUx3iMgS+m4hsGiMnUUOT5vHhUsw2RH85u+0X8/2b7CJE QjeZ+A/oOvOT29cfnp/vutCyzbaXH48Uu2cWdyOPnZLkz0wo5LtqKV+GK/1kOIFSaGPN RX79NypVBCHYgQe3TF+em9jCpzII7SKmuhhd2cIgJcVClmjHGkywEngrBjMdyfHUjg+H CVhQ== X-Gm-Message-State: APt69E2rpIr0jciXgXvayZCRv/7BcHbL2a3Wd6UtocbWNQVZsLGBmYEn LWS8JZxS5QD7bJpjQju5gWHnMN5NsCDw194zJU1v6Q== X-Google-Smtp-Source: AAOMgpf8CztqfLEWK8sz7l+bbEwCclcPJF8MxGx9KlKH+IK2JH0V7461OXicyibZVkzrKoPimBUUiO6+5zIzO0v+Nvc= X-Received: by 2002:adf:e9c1:: with SMTP id l1-v6mr15091822wrn.14.1531182642097; Mon, 09 Jul 2018 17:30:42 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:adf:f505:0:0:0:0:0 with HTTP; Mon, 9 Jul 2018 17:30:01 -0700 (PDT) In-Reply-To: <9160437f-6dbd-ec35-71a4-f6aa2a305881@redhat.com> References: <4e3b21af-4aeb-1524-df25-a1d7e08fdc94@redhat.com> <9160437f-6dbd-ec35-71a4-f6aa2a305881@redhat.com> From: Roman Bacik Date: Mon, 9 Jul 2018 17:30:01 -0700 Message-ID: To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Chao Zhang , Jiewen Yao , Vladimir Olovyannikov X-Content-Filtered-By: Mailman/MimeDel 2.1.27 Subject: Re: [PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Jul 2018 00:30:44 -0000 Content-Type: text/plain; charset="UTF-8" Laszlo, Thank you very much for your comments. I will address them and post another patch. Regards, Roman On Mon, Jul 9, 2018 at 5:24 PM, Laszlo Ersek wrote: > On 07/10/18 02:02, Laszlo Ersek wrote: > > On 07/10/18 00:11, Roman Bacik wrote: > > >> + PathName = AllocateZeroPool (PathLength); > >> + CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName, > >> PathLength); > > > > (3) I think it's not necessary to zero-fill the buffer, we're going to > > overwrite it right after the allocation. > > > > There's a convenience function for that: AllocateCopyPool(). > > > > (4) The number of bytes is not correct IMO. "PathLength" stands for the > > number of bytes in the entire device path node (FILEPATH_DEVICE_PATH), > > including Header and PathName. So, for getting the number of bytes in > > just PathName, we should subtract the size of Header. > > > > Presently, we over-read the source buffer; it's not causing problems > > because PathName is NUL-terminated. > > Sorry, I was unclear; I meant there were no *observable* problems. The > over-read of the source buffer results in garbage at the end of the > target buffer, which are later not consumed due to the terminating NUL > appearing earlier. Still, we should not over-read the source buffer. > > Thanks > Laszlo >