On Sun. 31 Jul. 2022 at 06:05, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 30.07.22 15:16, Vincent Mailhol wrote:
> > On Sat. 30 juil. 2022 at 21:06, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
>
> >>> Existing applications are exempted from this risk: the
> >>> CAN_RAW_XL_FRAMES socket option will prevent the collision from
> >>> happening. To summarise, the collision will only occur if below
> >>> criteria are met:
> >>>
> >>> * The user allocates a can_frame without zeroing it (typical use
> >>> cases are stack declaration or malloc()).
> >>> * The user activates the CAN_RAW_XL_FRAMES socket option.
> >>> * Garbage value results in can_frame::__pad & CANXL_XLF being set.
> >>>
> >>> So if going in that direction, we would have to update the
> >>> documentation (header files and kernel doc) to point at this pitfall
> >>> and specify that can_frame::__pad shall always be zeroed for mix
> >>> usages.
> >>
> >> But you would add this recommendation to a *very late point* - namely
> >> after 14 years of SocketCAN in the mainline kernel.
> >>
> >> If you think about the rule "never break user space" I wonder if the
> >> risk turns out to be higher when people enable CAN_RAW_XL_FRAMES ("as
> >> seen on Stackoverflow") and not read the kernel doc recommendations to
> >> review/update their existing code.
> >
> > In which aspect does this break the user space? It doesn't.
>
> It does in the way that you need to zero-initialize structures now.
>
> > Enabling CAN_RAW_XL_FRAMES marks a clear transition between the 14
> > years old Classical CAN API and the new use case of mixed CAN(-FD) and
> > CAN XL frames.
>
> You are seriously telling me that CAN_RAW_XL_FRAMES is a clear
> transition and people will automatically review and rework their former
> CAN/CANFD code to zero-initialize padding bytes?
>
> There is a much higher risk that this does not happen than telling
> people to check for CANXL_XLF first after reading from the socket!
>
> > For the issue to occur, developers have to incorrectly modify their
> > application, I do not call this a break in user space if only the
> > yet-to-be-created programs are impacted.
>
> I don't know about your thresholds but I call this a break in user
> space. Yet-to-be-created programs will likely copy old code.
>
> > The argument that people do not read the kernel doc apply to both
> > cases: some people will not initialise variables, some people will not
> > do the "if" checks in the right order.
>
> To make it simple: The difference between CAN XL and (old) CAN/CANFD
> handling is CANXL_XLF.
>
> I'm really tired about your constructed case that people would be too
> dumb to check for CANXL_XLF first.
>
> This CANFD_FDF check is a new optional(!) possibility, which we do not
> need to promote. This could become a power user trick for people that
> are obviously "smart enough" to put if-statements in the right order.
>
> The correct and safe check that should be documented and suggested looks
> like this:
>
> if (can.xl.flags & CANXL_XLF)
> /* Handle CAN XL frame */
> else if (nbytes == CANFD_MTU)
> /* Handle CAN FD frame */
> else if (nbytes == CAN_MTU) /* paranoid */
> /* Handle Classic CAN frame */
> else
> /* error */
After a good night of sleep, I found a use case which destroys my idea:
int main()
{
static union can can;
can.xl.flags = CANXL_XLF;
/* set other fields, open socket... */
write(s, &can.xl, CANXL_HD_SIZE + can.xl.len);
can.cc.len = 8;
/* set other field but leave can.cc._pad untouched */
write(s, &can.cc, sizeof(can.cc));
/* ... */
}
Here, we have a static declaration so the frame is guaranteed to be
zero initialized. The user then sends a CAN XL frame (and thus sets
the CANXL_XLF flag). Then the user sends a Classic CAN frame. However,
can.cc._pad still contains the CANXL_XLF flag set. I stay in my
position that asking to zero initialize variables for mix usage is
acceptable. But requiring to reset the can.cc._pad to zero in the
middle of the run time is too much.
This is a very bad foot gun. So let me withdraw my idea. Using the
0x80 flag on the can(fd)_frame.len is the best.
Yours sincerely,
Vincent Mailhol
