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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id A0D06C433EF for ; Sat, 23 Apr 2022 15:47:28 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 4E8A683D34; Sat, 23 Apr 2022 17:47:25 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com 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=gmail.com header.i=@gmail.com header.b="pII+bRgL"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 7C5F583DB7; Sat, 23 Apr 2022 17:47:22 +0200 (CEST) Received: from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com [IPv6:2a00:1450:4864:20::62b]) (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 C840783A79 for ; Sat, 23 Apr 2022 17:47:19 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=alpernebiyasak@gmail.com Received: by mail-ej1-x62b.google.com with SMTP id y20so21699682eju.7 for ; Sat, 23 Apr 2022 08:47:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=YyPGoQK6lAHCqkC/8HrvbL63stM1FihPCOVP0ily6oc=; b=pII+bRgLdvGIEb8xJlZdELLAWRAb75ra/LePX3EHOkfAmZJPrBTYg2HgoaCyihbmtJ dUWrLlzQqb0xUfCzMPMxdCcgeHPQfA4/iFsXPBHLbZFpbwEU7O74t+fEqv2t6LI/Vmm+ 30OMB6JFhRrLea/P9RRasmJXiyYCAiZfs3b34Ab5ygPYe/P9TNssQaGs8KIwf7uiSbDe eCcfxqQPfJwrg25+2rX742Me43X3tWfDEs3gtjeY4oTHkFWKORCkWX2SkSuYspSKNwgN ByRLR77JD92PSsGBrhS8nCjJ0Pfmr9M6nlI9HAhDqhYBzi+Vik/NZMxEGXLCwkttclBM dvWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=YyPGoQK6lAHCqkC/8HrvbL63stM1FihPCOVP0ily6oc=; b=24ttetJkrOwGmGHmyeqBzuDNY9O0VIYbfazIdjw5bpiUp00LegSNEp+ETT/ERLTIDs JCBYGk8b8TtwLYxvkTPnTKq4sdHvzeHSxBrrnnlMLC/K9iltUepInPTpdkVIvnJsBUfk s8gxCBT1MIrhjMyq2RnYD/o5HwaU+b8sEMAEXHhhFgT407M3WOliXqqh5HKj1Erxfwrp mphjlrS1d6wOZe3yIHv3Edf64+sbHQpZ8NfeT4eK9wNd43qqzakfm2oCzuQKKtzjmHjy p5hEBOdOLp8NQxRU/J10rzsxTNEEe6ulvZUmDpCSLw/BdKHkkb/l+86qMavQJO3JI3gL qtUA== X-Gm-Message-State: AOAM531pQp7qxvB5B3UyILusUmfajB6cD9sJ2e7uThtjogavoNHdOQ58 Igd4LYpV4HJF5oakHsF+4MI= X-Google-Smtp-Source: ABdhPJwvflbEOAvBic6VPhXuf+gRPJQ7O/fIyYGmdvQNySG1BnEsFy1N08O0U7UqjPwAFmto/CD6rw== X-Received: by 2002:a17:906:99c1:b0:6ef:d995:11ac with SMTP id s1-20020a17090699c100b006efd99511acmr8502854ejn.244.1650728839319; Sat, 23 Apr 2022 08:47:19 -0700 (PDT) Received: from [192.168.0.74] ([178.233.178.185]) by smtp.gmail.com with ESMTPSA id lb26-20020a170907785a00b006ea4d2928e5sm1772892ejc.218.2022.04.23.08.47.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 23 Apr 2022 08:47:18 -0700 (PDT) Message-ID: <150d615d-16ab-1998-701c-caa430a18c79@gmail.com> Date: Sat, 23 Apr 2022 18:46:57 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: Re: [PATCH 7/7] binman: Refuse to replace sections for now Content-Language: en-US To: Simon Glass Cc: U-Boot Mailing List , Heiko Thiery , Jan Kiszka References: <20220327153151.15912-1-alpernebiyasak@gmail.com> <20220327153151.15912-8-alpernebiyasak@gmail.com> From: Alper Nebi Yasak In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 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.5 at phobos.denx.de X-Virus-Status: Clean On 20/04/2022 00:54, Simon Glass wrote: > On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak wrote: >> >> Binman interfaces allow attempts to replace any entry in the image with >> arbitrary data. When trying to replace sections, the changes in the >> section entry's data are not propagated to its child entries. This, >> combined with how sections rebuild their contents from its children, >> eventually causes the replaced contents to be silently overwritten by >> rebuilt contents equivalent to the original data. >> >> Add a simple test for replacing a section that is currently failing due >> to this behaviour, and mark it as an expected failure. Also, raise an >> error when replacing a section instead of silently pretending it was >> replaced. >> >> Signed-off-by: Alper Nebi Yasak >> --- >> >> tools/binman/etype/section.py | 3 +++ >> tools/binman/ftest.py | 9 ++++++++ >> .../test/234_replace_section_simple.dts | 23 +++++++++++++++++++ >> 3 files changed, 35 insertions(+) >> create mode 100644 tools/binman/test/234_replace_section_simple.dts > > Reviewed-by: Simon Glass > > Is it not better to assertRaises() and check that the error message is > as expected? IMO, that would imply the tested 'binman replace' call should raise an error instead of replacing the data. The test should work as-is, and when section replacement is fixed we can just remove the expectedFailure line to be strict about it. > >> >> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py >> index ccac658c1831..bd67238b9199 100644 >> --- a/tools/binman/etype/section.py >> +++ b/tools/binman/etype/section.py >> @@ -788,6 +788,9 @@ def ReadChildData(self, child, decomp=True, alt_format=None): >> data = new_data >> return data >> >> + def WriteData(self, data, decomp=True): >> + self.Raise("Replacing sections is not implemented yet") >> + >> def WriteChildData(self, child): >> return True >> >> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py >> index 43bec4a88841..058651cc18a0 100644 >> --- a/tools/binman/ftest.py >> +++ b/tools/binman/ftest.py >> @@ -5641,6 +5641,15 @@ def testReplaceFitSubentryLeafSmallerSize(self): >> self.assertIsNotNone(path) >> self.assertEqual(expected_fdtmap, fdtmap) >> >> + @unittest.expectedFailure >> + def testReplaceSectionSimple(self): >> + """Test replacing a simple section with arbitrary data""" >> + new_data = b'w' * len(COMPRESS_DATA + U_BOOT_DATA) >> + data, expected_fdtmap, _ = self._RunReplaceCmd( >> + 'section', new_data, >> + dts='234_replace_section_simple.dts') >> + self.assertEqual(new_data, data) >> + >> >> if __name__ == "__main__": >> unittest.main() >> diff --git a/tools/binman/test/234_replace_section_simple.dts b/tools/binman/test/234_replace_section_simple.dts >> new file mode 100644 >> index 000000000000..c9d5c3285615 >> --- /dev/null >> +++ b/tools/binman/test/234_replace_section_simple.dts >> @@ -0,0 +1,23 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/dts-v1/; >> + >> +/ { >> + binman { >> + allow-repack; >> + >> + u-boot-dtb { >> + }; >> + >> + section { >> + blob { >> + filename = "compress"; >> + }; >> + >> + u-boot { >> + }; >> + }; >> + >> + fdtmap { >> + }; >> + }; >> +}; >> -- >> 2.35.1 >>