From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com [209.85.218.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D61733237 for ; Wed, 4 May 2022 19:58:15 +0000 (UTC) Received: by mail-ej1-f41.google.com with SMTP id y3so4873842ejo.12 for ; Wed, 04 May 2022 12:58:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=y+sByE56HbVf5kRX6gGxIKl4lkDcQYxYxjANal7cqo4=; b=Fisf8qNfLenDK0j792maHiYPMMvmyhGB9B3J/xS9stoAM/UZoSn3s9eWa4z58SqqtN zwNzc/F7jGucn3q+uOerwwvhkYcHR8WadSDg35wFqZ6iJOtuV3DgeIHYCsICPjn3FRC5 ijS3ruVDVcid8vwK6Aeyu8cvVWuqX4vHHcMprgjz8mGjIqaTTVh5y7xE1A9w5vPgc3/M 9OL1YFUN+X3Smqy7ONv67Xs9IxNQQJThHBH0mRzDjCqw6Y6Boyru+xViNBlwab5eTP66 Ql7OyvNLc8tFoVxvf1pFaiO/JKRt9GNoTAQHusID9bvyAzw/qXdqG5nBEcpTVWJq8YZ4 Z1Jg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=y+sByE56HbVf5kRX6gGxIKl4lkDcQYxYxjANal7cqo4=; b=jy5pIKJHiY34PpaC2jJ6z00MSGmpuPUtPa13HdyfmVHo8Lx0pQKaKPMYlv5Am7h6tn wmx1aCsic7l3zY3nCDezvXilKz+upmGNoejIR84FXdmg9YVySyBhVSW9fKYYvpRUtI5r qFioEFkdPFYjEBLIjpOT8VF68p/fne9WNdUKGeplxr0+oOcs3yqVCxpWiToyujDY8oO2 lFZkQsItz5kZ/ZX7FIZEW4ifiJ7Ijced9GzTmzJgvDF1Gz1KTveb+JNF/LwzyZN4IBR0 1PtQoyGYNz3hQYwK1Cm7mNmOS8tdwMB0XrWaG6mkze9wPlcefQ5rAtdwdYw9Dvelftuk 5bMQ== X-Gm-Message-State: AOAM533aioI2NWpHJEIpfbKpzOnCswufnSKzYA9YDxPR6029PAjr0i+v rHA6AsWKDNYkGQDwRYqnjVXcnMMt4Po0DUxpG7/qNA== X-Google-Smtp-Source: ABdhPJyNqylHJlc3dOLmSwn3LLwT4ktliGhTIZtJUbCp6p9cyFNeSQSMHaaSeuuF72ZsBGrGa7DMKOVMS/2gIc+sdec= X-Received: by 2002:a17:907:9726:b0:6f4:c0e:40ce with SMTP id jg38-20020a170907972600b006f40c0e40cemr21300141ejc.170.1651694293700; Wed, 04 May 2022 12:58:13 -0700 (PDT) Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20220504014440.3697851-1-keescook@chromium.org> <20220504014440.3697851-4-keescook@chromium.org> In-Reply-To: <20220504014440.3697851-4-keescook@chromium.org> From: Daniel Latypov Date: Wed, 4 May 2022 14:58:02 -0500 Message-ID: Subject: Re: [PATCH 03/32] flex_array: Add Kunit tests To: Kees Cook Cc: "Gustavo A . R . Silva" , David Gow , kunit-dev@googlegroups.com, Alexei Starovoitov , alsa-devel@alsa-project.org, Al Viro , Andrew Gabbasov , Andrew Morton , Andy Gross , Andy Lavr , Arend van Spriel , Baowen Zheng , Bjorn Andersson , Boris Ostrovsky , Bradley Grove , brcm80211-dev-list.pdl@broadcom.com, Christian Brauner , =?UTF-8?Q?Christian_G=C3=B6ttsche?= , Christian Lamparter , Chris Zankel , Cong Wang , Daniel Axtens , Daniel Vetter , Dan Williams , David Howells , "David S. Miller" , Dennis Dalessandro , devicetree@vger.kernel.org, Dexuan Cui , Dmitry Kasatkin , Eli Cohen , Eric Dumazet , Eric Paris , Eugeniu Rosca , Felipe Balbi , Francis Laniel , Frank Rowand , Franky Lin , Greg Kroah-Hartman , Gregory Greenman , Guenter Roeck , Haiyang Zhang , Hante Meuleman , Herbert Xu , Hulk Robot , Jakub Kicinski , "James E.J. Bottomley" , James Morris , Jarkko Sakkinen , Jaroslav Kysela , Jason Gunthorpe , Jens Axboe , Johan Hedberg , Johannes Berg , Johannes Berg , John Keeping , Juergen Gross , Kalle Valo , Keith Packard , keyrings@vger.kernel.org, Kuniyuki Iwashima , "K. Y. Srinivasan" , Lars-Peter Clausen , Lee Jones , Leon Romanovsky , Liam Girdwood , linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-bluetooth@vger.kernel.org, linux-hardening@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-integrity@vger.kernel.org, linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org, linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux-xtensa@linux-xtensa.org, llvm@lists.linux.dev, Loic Poulain , Louis Peens , Luca Coelho , Luiz Augusto von Dentz , Marc Dionne , Marcel Holtmann , Mark Brown , "Martin K. Petersen" , Max Filippov , Mimi Zohar , Muchun Song , Nathan Chancellor , netdev@vger.kernel.org, Nick Desaulniers , =?UTF-8?B?TnVubyBTw6E=?= , Paolo Abeni , Paul Moore , Rich Felker , Rob Herring , Russell King , selinux@vger.kernel.org, "Serge E. Hallyn" , SHA-cyfmac-dev-list@infineon.com, Simon Horman , Stefano Stabellini , Stefan Richter , Steffen Klassert , Stephen Hemminger , Stephen Smalley , Tadeusz Struk , Takashi Iwai , Tom Rix , Udipto Goswami , Vincenzo Frascino , wcn36xx@lists.infradead.org, Wei Liu , xen-devel@lists.xenproject.org, Xiu Jianfeng , Yang Yingliang Content-Type: text/plain; charset="UTF-8" On Tue, May 3, 2022 at 8:47 PM Kees Cook wrote: > +#define COMPARE_STRUCTS(STRUCT_A, STRUCT_B) do { \ > + STRUCT_A *ptr_A; \ > + STRUCT_B *ptr_B; \ > + int rc; \ > + size_t size_A, size_B; \ > + \ > + /* matching types for flex array elements and count */ \ > + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A), sizeof(*ptr_B)); \ > + KUNIT_EXPECT_TRUE(test, __same_type(*ptr_A->data, \ > + *ptr_B->__flex_array_elements)); \ Leaving some minor suggestions to go along with David's comments. Should we make these KUNIT_ASSERT_.* instead? I assume if we have a type-mismatch, then we should bail out instead of continuing to produce more error messages. > + KUNIT_EXPECT_TRUE(test, __same_type(ptr_A->datalen, \ > + ptr_B->__flex_array_elements_count)); \ > + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A->data), \ > + sizeof(*ptr_B->__flex_array_elements)); \ > + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), data), \ > + offsetof(typeof(*ptr_B), \ > + __flex_array_elements)); \ > + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), datalen), \ > + offsetof(typeof(*ptr_B), \ > + __flex_array_elements_count)); \ > + \ > + /* struct_size() vs __fas_bytes() */ \ > + size_A = struct_size(ptr_A, data, 13); \ > + rc = __fas_bytes(ptr_B, __flex_array_elements, \ > + __flex_array_elements_count, 13, &size_B); \ > + KUNIT_EXPECT_EQ(test, rc, 0); \ Hmm, what do you think about inlining the call/dropping rc? i.e. something like KUNIT_EXPECT_EQ(test, 0, __fas_bytes(ptr_B, __flex_array_elements, \ __flex_array_elements_count, 13, &size_B)); That would give a slightly clearer error message on failure. Otherwise the user only really gets a line number to try and start to understand what went wrong. > + > +#define CHECK_COPY(ptr) do { \ > + typeof(*(ptr)) *_cc_dst = (ptr); \ > + KUNIT_EXPECT_EQ(test, _cc_dst->induce_padding, 0); \ > + memcpy(&padding, &_cc_dst->induce_padding + sizeof(_cc_dst->induce_padding), \ > + sizeof(padding)); \ > + /* Padding should be zero too. */ \ > + KUNIT_EXPECT_EQ(test, padding, 0); \ > + KUNIT_EXPECT_EQ(test, src->count, _cc_dst->count); \ This also seems like a good place to use ASSERT instead of EXPECT. > + KUNIT_EXPECT_EQ(test, _cc_dst->count, TEST_TARGET); \ > + for (i = 0; i < _cc_dst->count - 1; i++) { \ > + /* 'A' is 0x41, and here repeated in a u32. */ \ > + KUNIT_EXPECT_EQ(test, _cc_dst->flex[i], 0x41414141); \ > + } \ > + /* Last item should be different. */ \ > + KUNIT_EXPECT_EQ(test, _cc_dst->flex[_cc_dst->count - 1], 0x14141414); \ > +} while (0) > + > +/* Test copying from one flexible array struct into another. */ > +static void flex_cpy_test(struct kunit *test) > +{ > +#define TEST_BOUNDS 13 > +#define TEST_TARGET 12 > +#define TEST_SMALL 10 > + struct flex_cpy_obj *src, *dst; > + unsigned long padding; > + int i, rc; > + > + /* Prepare open-coded source. */ > + src = kzalloc(struct_size(src, flex, TEST_BOUNDS), GFP_KERNEL); Looks like we could use kunit_kzalloc() here and avoid needing the manual call to kfree? This also holds for the other test cases where they don't have early calls to kfree(). Doing so would also let you use KUNIT_ASSERT's without fear of leaking these allocations. > + src->count = TEST_BOUNDS; > + memset(src->flex, 'A', flex_array_size(src, flex, TEST_BOUNDS)); > + src->flex[src->count - 2] = 0x14141414; > + src->flex[src->count - 1] = 0x24242424; > + > + /* Prepare open-coded destination, alloc only. */ > + dst = kzalloc(struct_size(src, flex, TEST_BOUNDS), GFP_KERNEL); > + /* Pre-fill with 0xFE marker. */ > + memset(dst, 0xFE, struct_size(src, flex, TEST_BOUNDS)); > + /* Pretend we're 1 element smaller. */ > + dst->count = TEST_TARGET; > + > + /* Pretend to match the target destination size. */ > + src->count = TEST_TARGET; > + > + rc = flex_cpy(dst, src); > + KUNIT_EXPECT_EQ(test, rc, 0); > + CHECK_COPY(dst); > + /* Item past last copied item is unchanged from initial memset. */ > + KUNIT_EXPECT_EQ(test, dst->flex[dst->count], 0xFEFEFEFE); > + > + /* Now trip overflow, and verify we didn't clobber beyond end. */ > + src->count = TEST_BOUNDS; > + rc = flex_cpy(dst, src); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + /* Item past last copied item is unchanged from initial memset. */ > + KUNIT_EXPECT_EQ(test, dst->flex[dst->count], 0xFEFEFEFE); > + > + /* Reset destination contents. */ > + memset(dst, 0xFD, struct_size(src, flex, TEST_BOUNDS)); > + dst->count = TEST_TARGET; > + > + /* Copy less than max. */ > + src->count = TEST_SMALL; > + rc = flex_cpy(dst, src); > + KUNIT_EXPECT_EQ(test, rc, 0); > + /* Verify count was adjusted. */ > + KUNIT_EXPECT_EQ(test, dst->count, TEST_SMALL); Just an FYI, macros get evaluated before the expect macros can stringify them. So the error message would look something like Expected dest->count == 10 but dest->count = 9 Not a big concern, but just noting that "TEST_SMALL" won't be visible at all. Could opt for KUNIT_EXPECT_EQ_MSG(test, dst->count, TEST_SMALL, "my custom extra message"); if you think it'd be usable to make the test more grokkable. > + /* Verify element beyond src size was wiped. */ > + KUNIT_EXPECT_EQ(test, dst->flex[TEST_SMALL], 0); > + /* Verify element beyond original dst size was untouched. */ > + KUNIT_EXPECT_EQ(test, dst->flex[TEST_TARGET], 0xFDFDFDFD); > + > + kfree(dst); > + kfree(src); > +#undef TEST_BOUNDS > +#undef TEST_TARGET > +#undef TEST_SMALL > +} > + > +static void flex_dup_test(struct kunit *test) > +{ > +#define TEST_TARGET 12 > + struct flex_cpy_obj *src, *dst = NULL, **null = NULL; > + struct flex_dup_obj *encap = NULL; > + unsigned long padding; > + int i, rc; > + > + /* Prepare open-coded source. */ > + src = kzalloc(struct_size(src, flex, TEST_TARGET), GFP_KERNEL); > + src->count = TEST_TARGET; > + memset(src->flex, 'A', flex_array_size(src, flex, TEST_TARGET)); > + src->flex[src->count - 1] = 0x14141414; > + > + /* Reject NULL @alloc. */ > + rc = flex_dup(null, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + > + /* Check good copy. */ > + rc = flex_dup(&dst, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_ASSERT_TRUE(test, dst != NULL); > + CHECK_COPY(dst); > + > + /* Reject non-NULL *@alloc. */ > + rc = flex_dup(&dst, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + > + kfree(dst); > + > + /* Check good encap copy. */ > + rc = __flex_dup(&encap, .fas, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_ASSERT_TRUE(test, dst != NULL); FYI, there's a new KUNIT_ASSERT_NOT_NULL() macro in the -kselftest/kunit branch, https://patchwork.kernel.org/project/linux-kselftest/patch/20220211164246.410079-1-ribalda@chromium.org/ But that's not planned for inclusion into mainline until 5.19, so leaving this as-is is better for now.