| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAM0EoMnWCTtpVjtgErubwGNuimgc5wP9MY2G7S2dPTr5rXvKsw@mail.gmail.com>
Date: Sat, 3 Jun 2023 09:54:48 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Marcelo Ricardo Leitner <mleitner@...hat.com>
Cc: netdev@...r.kernel.org, deb.chatterjee@...el.com, anjali.singhai@...el.com,
namrata.limaye@...el.com, tom@...anda.io, p4tc-discussions@...devconf.info,
Mahesh.Shirshyad@....com, Vipin.Jain@....com, tomasz.osinski@...el.com,
jiri@...nulli.us, xiyou.wangcong@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, vladbu@...dia.com,
simon.horman@...igine.com, khalidm@...dia.com, toke@...hat.com
Subject: Re: [PATCH RFC v2 net-next 09/28] p4tc: add P4 data types
On Fri, Jun 2, 2023 at 4:30 PM Marcelo Ricardo Leitner
<mleitner@...hat.com> wrote:
>
> On Wed, May 17, 2023 at 07:02:13AM -0400, Jamal Hadi Salim wrote:
> > +bool p4tc_type_unsigned(int typeid)
>
> Nit, maybe name it p4tc_is_type_unsigned() instead.
>
Yeah, that sounds better... we'll make the change.
> > +{
> > + switch (typeid) {
> > + case P4T_U8:
> > + case P4T_U16:
> > + case P4T_U32:
> > + case P4T_U64:
> > + case P4T_U128:
> > + case P4T_BOOL:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +int p4t_copy(struct p4tc_type_mask_shift *dst_mask_shift,
> > + struct p4tc_type *dst_t, void *dstv,
> > + struct p4tc_type_mask_shift *src_mask_shift,
> > + struct p4tc_type *src_t, void *srcv)
> > +{
> > + u64 readval[BITS_TO_U64(P4TC_MAX_KEYSZ)] = {0};
> > + const struct p4tc_type_ops *srco, *dsto;
> > +
> > + dsto = dst_t->ops;
> > + srco = src_t->ops;
> > +
> > + __p4tc_type_host_read(srco, src_t, src_mask_shift, srcv,
> > + &readval);
> > + __p4tc_type_host_write(dsto, dst_t, dst_mask_shift, &readval,
> > + dstv);
> > +
> > + return 0;
>
> The return value on these (write) functions seems to be inconsistent.
> All the write functions are returning 0. Then, __p4tc_type_host_write
> itself propagates the return value, but then here it doesn't.
Yeah, mostly these should not "fail" (famous last words) due to
memcpy. We started by looking at the return of memcpy - which returns
the pointer to dest but is known to fail under some circumstances eg
buffers overlap due to some bug on the caller side; so to be safe at
the time was to do the check and return a failure when detected. But
then we argued amongst ourselves and decided not to check. So what you
are seeing is a result of that and there is some leftover you caught
in your inspection. We could either go back and add the check or
totally remove it to make it return void. Thoughts?
> > +}
> > +
> > +int p4t_cmp(struct p4tc_type_mask_shift *dst_mask_shift,
> > + struct p4tc_type *dst_t, void *dstv,
> > + struct p4tc_type_mask_shift *src_mask_shift,
> > + struct p4tc_type *src_t, void *srcv)
> > +{
> > + u64 a[BITS_TO_U64(P4TC_MAX_KEYSZ)] = {0};
> > + u64 b[BITS_TO_U64(P4TC_MAX_KEYSZ)] = {0};
> > + const struct p4tc_type_ops *srco, *dsto;
> > +
> > + dsto = dst_t->ops;
> > + srco = src_t->ops;
> > +
> > + __p4tc_type_host_read(dsto, dst_t, dst_mask_shift, dstv, a);
> > + __p4tc_type_host_read(srco, src_t, src_mask_shift, srcv, b);
> > +
> > + return memcmp(a, b, sizeof(a));
> > +}
> > +
> > +void p4t_release(struct p4tc_type_mask_shift *mask_shift)
> > +{
> > + kfree(mask_shift->mask);
> > + kfree(mask_shift);
> > +}
> > +
> > +static int p4t_validate_bitpos(u16 bitstart, u16 bitend, u16 maxbitstart,
> > + u16 maxbitend, struct netlink_ext_ack *extack)
> > +{
> > + if (bitstart > maxbitstart) {
> > + NL_SET_ERR_MSG_MOD(extack, "bitstart too high");
> > + return -EINVAL;
> > + }
> > + if (bitend > maxbitend) {
> > + NL_SET_ERR_MSG_MOD(extack, "bitend too high");
> > + return -EINVAL;
> > + }
>
> Do we want a condition for
> + if (bitstart > bitend) {
> + NL_SET_ERR_MSG_MOD(extack, "bitstart after bitend");
> + return -EINVAL;
> + }
> ?
We'll audit the code again - but we normally check for that condition
at the caller level (eg check patch 17, validate_metadata_operand())
It may be worth doing it here instead of at the caller.
> > +
> > + return 0;
> > +}
> > +
> > +//XXX: Latter immedv will be 64 bits
> > +static int p4t_u32_validate(struct p4tc_type *container, void *value,
> > + u16 bitstart, u16 bitend,
> > + struct netlink_ext_ack *extack)
> > +{
> > + u32 container_maxsz = U32_MAX;
> > + u32 *val = value;
> > + size_t maxval;
> > + int ret;
> > +
> > + ret = p4t_validate_bitpos(bitstart, bitend, 31, 31, extack);
> > + if (ret < 0)
> > + return ret;
> > +
> > + maxval = GENMASK(bitend, 0);
> > + if (val && (*val > container_maxsz || *val > maxval)) {
> > + NL_SET_ERR_MSG_MOD(extack, "U32 value out of range");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct p4tc_type_mask_shift *
> > +p4t_u32_bitops(u16 bitsiz, u16 bitstart, u16 bitend,
> > + struct netlink_ext_ack *extack)
> > +{
> > + u32 mask = GENMASK(bitend, bitstart);
> > + struct p4tc_type_mask_shift *mask_shift;
> > + u32 *cmask;
> > +
> > + mask_shift = kzalloc(sizeof(*mask_shift), GFP_KERNEL);
> > + if (!mask_shift)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + cmask = kzalloc(sizeof(u32), GFP_KERNEL);
> > + if (!cmask) {
> > + kfree(mask_shift);
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + *cmask = mask;
> > +
> > + mask_shift->mask = cmask;
> > + mask_shift->shift = bitstart;
>
> AFAICT, mask_shift->mask is never shared. So maybe consider
> embedding mask onto mask_shift itself, to avoid the double allocation.
> I mean, something like
> + mask_shift = kzalloc(sizeof(*mask_shift)+sizeof(u32), GFP_KERNEL);
> + cmask = mask_shift+1;
>
> This may also help with cache miss later on.
It's a lot of mechanical work for us to do the conversion but it's
valuable if we can avoid the cache miss. We'll look into it - it may
not look exactly as what you have suggested but given we know exact
size so we can do some optimal things.
cheers,
jamal
> > +
> > + return mask_shift;
> > +}
>
Powered by blists - more mailing lists