From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
To: Dan Carpenter <dan.carpenter@linaro.org>,
oe-kbuild@lists.linux.dev, Vadim Fedorenko <vadfed@meta.com>,
Jakub Kicinski <kuba@kernel.org>,
Martin KaFai Lau <martin.lau@linux.dev>,
Andrii Nakryiko <andrii@kernel.org>,
Alexei Starovoitov <ast@kernel.org>,
Mykola Lysenko <mykolal@fb.com>,
Herbert Xu <herbert@gondor.apana.org.au>
Cc: lkp@intel.com, oe-kbuild-all@lists.linux.dev,
netdev@vger.kernel.org, linux-crypto@vger.kernel.org,
bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v7 1/3] bpf: make common crypto API for TC/XDP programs
Date: Thu, 7 Dec 2023 12:14:12 +0000 [thread overview]
Message-ID: <098fb386-0309-4313-866a-38e12b54c02a@linux.dev> (raw)
In-Reply-To: <dc0e2f8e-f82b-4439-b61a-9ab0be9f4e6b@suswa.mountain>
On 05/12/2023 21:56, Dan Carpenter wrote:
> Hi Vadim,
>
> kernel test robot noticed the following build warnings:
>
> url: https://github.com/intel-lab-lkp/linux/commits/Vadim-Fedorenko/bpf-crypto-add-skcipher-to-bpf-crypto/20231202-091254
> base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> patch link: https://lore.kernel.org/r/20231202010604.1877561-1-vadfed%40meta.com
> patch subject: [PATCH bpf-next v7 1/3] bpf: make common crypto API for TC/XDP programs
> config: x86_64-randconfig-161-20231202 (https://download.01.org/0day-ci/archive/20231206/202312060647.2JfAE3rk-lkp@intel.com/config)
> compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
> reproduce: (https://download.01.org/0day-ci/archive/20231206/202312060647.2JfAE3rk-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202312060647.2JfAE3rk-lkp@intel.com/
>
> smatch warnings:
> kernel/bpf/crypto.c:192 bpf_crypto_ctx_create() error: we previously assumed 'ctx' could be null (see line 165)
> kernel/bpf/crypto.c:192 bpf_crypto_ctx_create() error: potentially dereferencing uninitialized 'ctx'.
>
> vim +/ctx +192 kernel/bpf/crypto.c
>
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 122 __bpf_kfunc struct bpf_crypto_ctx *
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 123 bpf_crypto_ctx_create(const char *type__str, const char *algo__str,
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 124 const struct bpf_dynptr_kern *pkey,
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 125 unsigned int authsize, int *err)
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 126 {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 127 const struct bpf_crypto_type *type = bpf_crypto_get_type(type__str);
>
> Delete this assignment. (Duplicated).
>
Ah, yeah, will remove it.
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 128 struct bpf_crypto_ctx *ctx;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 129 const u8 *key;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 130 u32 key_len;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 131
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 132 type = bpf_crypto_get_type(type__str);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 133 if (IS_ERR(type)) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 134 *err = PTR_ERR(type);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 135 return NULL;
>
> Why doesn't this function just return error pointers?
bpf_kfuncs cannot return error pointers, it makes BPF verifier very unhappy.
>
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 136 }
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 137
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 138 if (!type->has_algo(algo__str)) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 139 *err = -EOPNOTSUPP;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 140 goto err;
>
> ctx is uninitialized on this path.
>
Yep, it was already highlighted in the feedback, thanks.
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 141 }
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 142
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 143 if (!authsize && type->setauthsize) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 144 *err = -EOPNOTSUPP;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 145 goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 146 }
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 147
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 148 if (authsize && !type->setauthsize) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 149 *err = -EOPNOTSUPP;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 150 goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 151 }
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 152
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 153 key_len = __bpf_dynptr_size(pkey);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 154 if (!key_len) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 155 *err = -EINVAL;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 156 goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 157 }
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 158 key = __bpf_dynptr_data(pkey, key_len);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 159 if (!key) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 160 *err = -EINVAL;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 161 goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 162 }
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 163
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 164 ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 @165 if (!ctx) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 166 *err = -ENOMEM;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 167 goto err;
>
> ctx is NULL here.
>
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 168 }
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 169
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 170 ctx->type = type;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 171 ctx->tfm = type->alloc_tfm(algo__str);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 172 if (IS_ERR(ctx->tfm)) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 173 *err = PTR_ERR(ctx->tfm);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 174 ctx->tfm = NULL;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 175 goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 176 }
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 177
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 178 if (authsize) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 179 *err = type->setauthsize(ctx->tfm, authsize);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 180 if (*err)
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 181 goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 182 }
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 183
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 184 *err = type->setkey(ctx->tfm, key, key_len);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 185 if (*err)
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 186 goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 187
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 188 refcount_set(&ctx->usage, 1);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 189
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 190 return ctx;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 191 err:
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 @192 if (ctx->tfm)
> ^^^^^^^^
> NULL dereference. These two error handling bugs in three lines of code
> are canonical One Err Label type bugs. Better to do a ladder where each
> error label frees the last thing that was allocated. Easier to review.
> Then you could delete the "ctx->tfm = NULL;" assignment on line 174.
>
> return ctx;
>
> err_free_tfm:
> type->free_tfm(ctx->tfm);
> err_free_ctx:
> kfree(ctx);
> err_module_put:
> module_put(type->owner);
>
> return NULL;
>
> I have written about this at length on my blog:
> https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/
Thanks, very good blog post, I'll follow this way in the next version.
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 193 type->free_tfm(ctx->tfm);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 194 kfree(ctx);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 195 module_put(type->owner);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 196
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 197 return NULL;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 198 }
>
next prev parent reply other threads:[~2023-12-07 12:14 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-02 1:06 [PATCH bpf-next v7 1/3] bpf: make common crypto API for TC/XDP programs Vadim Fedorenko
2023-12-02 1:06 ` [PATCH bpf-next v7 2/3] bpf: crypto: add skcipher to bpf crypto Vadim Fedorenko
2023-12-02 3:52 ` Herbert Xu
2023-12-03 20:00 ` Vadim Fedorenko
2023-12-02 1:06 ` [PATCH bpf-next v7 3/3] selftests: bpf: crypto skcipher algo selftests Vadim Fedorenko
2023-12-03 10:59 ` Simon Horman
2023-12-03 18:43 ` Vadim Fedorenko
2023-12-05 1:28 ` Martin KaFai Lau
2023-12-02 1:48 ` [PATCH bpf-next v7 1/3] bpf: make common crypto API for TC/XDP programs Martin KaFai Lau
2023-12-03 19:02 ` Vadim Fedorenko
2023-12-04 23:08 ` Martin KaFai Lau
2023-12-03 10:57 ` Simon Horman
2023-12-03 19:08 ` Vadim Fedorenko
2023-12-05 20:19 ` kernel test robot
2023-12-05 21:15 ` kernel test robot
2023-12-06 5:56 ` Dan Carpenter
2023-12-07 12:14 ` Vadim Fedorenko [this message]
-- strict thread matches above, loose matches on Subject: below --
2023-12-05 22:19 kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=098fb386-0309-4313-866a-38e12b54c02a@linux.dev \
--to=vadim.fedorenko@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=dan.carpenter@linaro.org \
--cc=herbert@gondor.apana.org.au \
--cc=kuba@kernel.org \
--cc=linux-crypto@vger.kernel.org \
--cc=lkp@intel.com \
--cc=martin.lau@linux.dev \
--cc=mykolal@fb.com \
--cc=netdev@vger.kernel.org \
--cc=oe-kbuild-all@lists.linux.dev \
--cc=oe-kbuild@lists.linux.dev \
--cc=vadfed@meta.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.