netdev - Re: [RFC PATCH v1 0/1] net: mctp: MCTP VDM extension

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aceee8aa24b33f99ebf22cf02d2063b9e3b17bb3.camel@codeconstruct.com.au>
Date:   Wed, 26 Apr 2023 18:24:23 +0800
From:   Jeremy Kerr <jk@...econstruct.com.au>
To:     Krzysztof Richert <krzysztof.richert@...el.com>,
        matt@...econstruct.com.au
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [RFC PATCH v1 0/1] net: mctp: MCTP VDM extension

Hi Krzysztof,

> According to vendor needs it seems that supporting Vendor Define
> Messages based on  PCI vendor ID (0x7e) or based on IANA number (0x7f)
> which are described in DSP0236 will bring additional value to existing
> MCTP susbsystem. Currently MCTP subsystem allows to register one
> specific client for each MCTP message type.

Excellent, thanks for taking a look at this.

I have some comments on the general structure though. I'll discuss those
here, and we can cover the implementation details once that's sorted.

> Under MCTP type=0x7e we handle two different messages, one is handle
> by user-space application and the other is handle by kernel-space
> driver.

OK, but the kernel vs. user distinction has no influence on the
addressing, right?

> Because each vendor may define, own, internal message format 
> that's why we considered to extend sockaddr_mctp and allow for
> registration on MCTP message types = 0x7e/0x7f with additional
> sub-type (up to 8 bytes), which can be defined and use by each vendor
> to distinguish MCTP VDM packages.

So there are really three components here:

 1) the existing type byte; in this case, either 0x7e/0x7f for
    indicating PCI/IANA vendor types

 2) the PCI (2-byte) or IANA (4-byte) vendor identifier

 3) an additional sub-type identifier

While (1) and (2) are well-defined by DSP0236, my concern is that (3) is
not specified anywhere, and seems to be an entirely Intel-specific
construct. Or is there something in the pipeline for the spec here?

Given you're proposing special handling for Intel vendor types, this
implies that we're going to be changing the addressing mechanism for
every new vendor-based type.

So - a couple of questions to shape the design:

 - do you *really* need the sub-type decoding in the kernel? (could it be
   possible for one bind() to handle all of a specific vendor type
   instead? or is this related to the kernel vs. user statement above?)

   if you do need it:

 - could we turn this into a non-vendor-specific (length, data) match
   instead? If the length is zero, this falls back to exactly as
   specified in DSP0236.

   or:

 - is there a better way of filtering the subtypes into each bound
   socket?

on the uapi:

> union mctp_vendor_id {
>         struct {
>                 __u32           resvd   : 16,
>                                         data    : 16;
>         } pci_vendor_id;

no bitfields please - just two __u16 fields instead.

> 
>         struct {
>                 __u32           data;
>         } iana_number;
> };
> 
> 
> struct mctp_vdm_data {
>         union mctp_vendor_id            smctp_vendor;
>         __u64                   __smctp_pad0;

This padding is a bit mysterious - what are you trying to pad here?

>         __u64                   smctp_sub_type;

As above: if we can turn this into separate length & value fields, we
can avoid having to code every vendor format into the kernel.

Also, do we really need 8 bytes of type for this? Is some vendor
planning to support more than 4.3 billion MCTP subtypes? :)

> };
> 
> struct sockaddr_mctp_vendor_ext {
>         struct sockaddr_mctp_ext        smctp_ext;
>         struct mctp_vdm_data smctp_vdm_data;
> };

You could avoid a bit of wrapping by including the ext fields directly
into the vendor-enabled sockaddr:

    struct sockaddr_mctp_vendor_ext {
    	struct sockaddr_mctp	smctp_base;
    	int			smctp_ifindex;
    	__u8			smctp_halen;
    	__u8			__smctp_pad0[3];
    	__u8			smctp_haddr[MAX_ADDR_LEN];
        struct mctp_vdm_data	smctp_vdm_data;
    };

- but we would need to ensure that we're exactly matching the layout of
sockaddr_mctp_ext; we could do that through a few build-time asserts
though.

We would probably simplify this a little too, by flattening into:

    struct sockaddr_mctp_vendor_ext {
    	struct sockaddr_mctp	smctp_base;
    	int			smctp_ifindex;
    	__u8			smctp_halen;
    	__u8			__smctp_pad0[3];
    	__u8			smctp_haddr[MAX_ADDR_LEN];
        __u32			smctp_vdm_vendor;
        __u32			smctp_vdm_type;
        __u32			smctp_vdm_len;
    };

but it's worthwhile working out the actual addressing semantics before
defining this.

We'll also have to be very explicit about the endianness here.

This is also under the assumption that we want to be able to support
both extended addressing *and* vendor addressing at the same time. I
don't think there's any reason not to, but any thoughts on that?

Cheers,


Jeremy