On Wed. 13 Jul. 2022 at 04:24, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 12.07.22 16:31, Vincent Mailhol wrote:
> > On Tue. 12 Jul. 2022 at 21:30, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> >> On 12.07.22 12:19, Vincent Mailhol wrote:
> >>> On Tue. 12 Jul. 2022 at 18:31, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> >>
> >>>>>> +/* truncated CAN XL structs must contain at least 64 data bytes */
> >>>>>> +#define CANXL_MINTU (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)
> >>>>>
> >>>>> I did not get the concept of the "truncated CAN XL structs". The valid
> >>>>> data field lengths are 1 to 2048, right? I did not get where this 64
> >>>>> comes from.
> >>>>> Your formula is equivalent to
> >>>>> #define CANXL_MINTU (sizeof(struct canxl_frame) + CANFD_MAX_DLEN)
> >>>>
> >>>> No. CANXL_MINTU becomes sizeof(struct canfd_frame) + sizeof(af)
> >>>>
> >>>> So I wanted some size value that is 'more than' CANFD_MTU so that you
> >>>> know that you have read a CANXL frame.
> >>>>
> >>>> Even if the cxf->len would be 14 you would at least copy a struct
> >>>> canxl_frame with data[64].
> >>>
> >>> OK, I finally got your point. Your concern is that if skb->len could
> >>> be equal or less than CANFD_MTU, then there would be a collision.
> >>>
> >>> My approach here would be to stop using the MTU correlation to
> >>> differentiate between CAN(-FD) and CANXL. Instead, I suggest using
> >>> can{fd,xl}_frame::flags. If can{fd,xl}_frame has a CANXL flag set,
> >>> then it is a CANXL frame regardless of the value of skb->len. If the
> >>> CANXL flag is not set, then skb->len is used to differentiate between
> >>> Classic CAN and CAN FD (so that we remain compatible with the
> >>> existing). That way, no need to impose a minimum length of
> >>> CANFD_MAX_DLEN.
> >>
> >> Hm, that sounds interesting! I like that as it looks clear and simple.
> >>
> >> Need to pick a coffee
> >
> > Two years ago, it was a beer:
> > https://lore.kernel.org/linux-can/a9605011-2674-dc73-111c-8ebf724a13ac@xxxxxxxxxxxx/
> >
> > Now it is coffee. Seems that you change your drinking habits (just kidding) ;-)
>
> That depends on the daytime ;-)
>
> In Germany there's a saying:
> "Kein Bier vor vier!"
> (no beer before 4pm)
>
> But a friend of my dad has a clock in his garage with only '4's on the
> watchface to make this test always return true :-D
He, he, he! I had an argument with a German friend a long time ago. We
concluded that I was always past 4 p.m. somewhere in the world. This
works especially great for imported beers which you can drink at the
foreign timezone (to pay homage of course!)
> >> to think about potential (security) effects ... ;-)
> >
> > If we require a socket option as you already did (c.f.
> > CAN_RAW_XL_FRAMES), I do not see a security risk.
> >
> > If not using the socket option, then a user who activates CAN_XL at
> > the netlink level and who runs a legacy application in which struct
> > can_frame is declared on the stack and not initialised would be at
> > risk. can_frame::__pad could contain garbage data which could be
> > interpreted as a valid CAN_XL flag. From that point, the garbage
> > values in can_frame::__res0 and can_frame::len8_dlc would be
> > interpreted as an CANXL length.
> >
> > The only requirement is that all applications which will use a mix of
> > CANXL and non-CANXL frames shall make sure that no garbage values are
> > present in can_frame::__pad or canfd_frame::flags. Coding guidelines
> > such as MISRA and good static analyzers should also be able to catch
> > this.
>
> Yes. This was exactly my concern. But we can not assume that user space
> application is friendly or follows any MISRA guidelines.
Well, if we document than can_frame::__pad shall be zero for mix
usages (i.e. comments in struct can_frame and updated kernel doc),
then we would have done our due diligence. From that point, if people
ignore the documentation *and* do not follow best practices for safety
application development, I wouldn't cry.
If we absolutely want to prevent struct can_frame to be interpreted as
a canxl_frame due to some stack garbage, we can add one
padding/reserved field like that:
struct canxl_frame {
canid_t prio; /* 11 bit priority for arbitration (canid_t) */
__u8 sdt; /* SDU (service data unit) type */
__u8 flags; /* additional flags for CAN XL */
__u16 len; /* frame payload length in byte */
__u32 af; /* acceptance field */
__u32 __res; /* reserved field. Shall be zero */
__u8 data[] __attribute__((aligned(8)));
};
This way, the minimum transfer unit of CANXL is 17 bytes (16 for
header and 1 for data) which is exactly one byte more than can_frame
(and we get back the 8 bytes alignment \o/)
This would only leave the risk of having some garbage in
canfd_frame::flags, e.g. if user does:
struct canfd_frame cfd; /* declared on the stack and not initialized */
cfd.flags |= <some_flags> /* use |= instead of = */
But this is already risky for plain CAN-FD.
> Following your flexible length with CANXL_XLF idea you may forge a CAN
> XL frame inside a Classical CAN frame. But then we simply need to treat
> it as CAN XL frame any apply the other sanity checks.
>
> >> E.g. we would need to keep skb->len and canxl_frame::len in sync now.
> >
> > Not necessarily. The only strong condition is that:
> > skb_buff::len + sizeof(struct canxl_frame) < canxl_frame::len
> >
> > If it is equal, then perfect, we are optimal. If greater, it just
> > means that there is some garbage data. If the condition is not met, we
> > just drop the skb of course.
> >
> > Technically, we could remove canxl_frame::len and use below formula to
> > derive the data length:
> > len = skb_buff::len - sizeof(struct canxl_frame)
> >
> > In that case, the user must do all the length calculations correctly.
> > This would be close to how TCP/IP frames are managed. But personally,
> > I do not recommend removing canxl_frame::len.
> >
> > Having both skb_buff::len and canxl_frame::len in sync is a design
> > choice, not a necessity. I am still thinking of the implications and
> > what is best between allowing garbage or forcing the two lengths to be
> > in sync.
>
> Ok.
>
> (..)
>
> >> I still would suggest to have the struct canxl_frame contain the 2048
> >> byte of data (data[CANXL_MAXDLEN]) - as the entire CAN XL frame is
> >> defined like this in the CAN XL spec. This would be also in common with
> >> CAN/CANFD.
> >>
> >> E.g. when reading into the struct canxl_frame you always have a defined
> >> data structure which can contain a complete CAN XL frame.
> >
> > If we go this way, then I would allow the user to put garbage (i.e.
> > not having the two lengths in sync).
> > The rationale would be that we actually already allow such garbage
> > values for CAN and CAN-FD. Also, this way, the user who has zero clues
> > about the flexible array member property would simply do:
> >
> > struct canxl_frame cxl = { 0 };
> > /* ... */
>
> (len and CANXL_XLF would have needed to be set here)
ACK.
> > write(socket, &cxl, sizeof(cxl));
> >
> > and it would work.
>
> Yes.
>
>
> > The advanced user who understand what is going on
> > can still do:
> >
> > struct canxl_frame cxl = { 0 };
> > /* ... */
> > write(socket, &cxl, CANXL_HEAD_SZ + cxl.len);
> >
>
> ACK. I think this is feasible and easy to understand.
>
> >> But if you get or send less than that size (when reading/writing) this
> >> would be ok now (with your idea with CANXL_XLF set).
> >>
> >>
> >> E.g.
> >>
> >> #define CANXL_MINDLEN 1
> >> #define CANXL_MAXDLEN 2048
> >
> > To be consistent with CAN_DLEN and CANFD_DLEN names:
> > #define CANXL_DLEN 2048
>
> You are right about the inconsistency. But it needs to be
>
> #define CANXL_MIN_DLEN 1
> #define CANXL_MAX_DLEN 2048
>
> to fit the CAN/CANFD definitions.
ACK.
> >> #define CANXL_MTU sizeof(struct canxl_frame)
> >> #define CANXL_HEAD_SZ (CANXL_MTU - CANXL_MAXDLEN)
> >> #define CANXL_MINTU (CANXL_HEAD_SZ + CANXL_MINDLEN)
> >
> > I need to think twice about all that, all the different alternatives
> > (allow or not garbage, data as flexible member array vs.
> > data[CANXL_DLEN]). Now it is a bit late, so I will continue to think
> > about all that tomorrow.
>
> I would suggest to not allow garbage and have skbuff::len and
> canxl_frame::len in sync.
>
> When a userspace application writes more bytes than needed by
> canxl_frame::len, then that garbage is cropped to the needed size.
Cropped by whom? My point is to allow userspace to leave garbage. I am
totally OK with cropping the frames as soon as they enter the kernel.
On the other hand, the kernel shall always return cropped frames to
the userland. To summarize, skb and data len synchronisation is
optional for userland but mandatory for kernel drivers.
> When less data is provided than required by canxl_frame::len this could
> lead to an error.
>
> > But overall, I like the direction this thread is taking ;-)
>
> Yep!
>
> (..)
Yours sincerely,
Vincent Mailhol
