linux-kernel - Re: [PATCH 3/3] serial: Add kserial_rs485 to avoid wasted space due to .padding

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Aug 2022 18:50:54 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Jonathan Corbet <corbet@....net>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Russell King <linux@...linux.org.uk>,
        Richard Genoud <richard.genoud@...il.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Linux Documentation List <linux-doc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        linux-stm32@...md-mailman.stormreply.com,
        Lino Sanfilippo <LinoSanfilippo@....de>
Subject: Re: [PATCH 3/3] serial: Add kserial_rs485 to avoid wasted space due
 to .padding

On Fri, Aug 26, 2022 at 5:51 PM Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> Struct serial_rs485 has a .padding field to make uapi updates easier.

The struct

> It wastes space, however. Create struct kserial_rs485 which is a kerner
> counterpart w/o padding.
>
> kernel_serial_rs485_to_user_rs485()'s rs485 can now become const as
> padding is dealt within the local variable.

...

> -static int user_rs485_to_kernel_serial_rs485(struct serial_rs485 *rs485,
> +static int user_rs485_to_kernel_serial_rs485(struct kserial_rs485 *rs485,
>                                              const struct serial_rs485 __user *rs485_user)
>  {
> -       if (copy_from_user(rs485, rs485_user, sizeof(*rs485)))
> +       struct serial_rs485 rs485_uapi;
> +
> +       if (copy_from_user(&rs485_uapi, rs485_user, sizeof(*rs485)))
>                 return -EFAULT;

> +       *rs485 = *((struct kserial_rs485 *)&rs485_uapi);

So with all assets we have we can be sure that on BE64 / BE32 machines
this will be flawless. Is this assumption correct?

>         return 0;
>  }

...

>  static int kernel_serial_rs485_to_user_rs485(struct serial_rs485 __user *rs485_user,
> -                                            struct serial_rs485 *rs485)
> +                                            const struct kserial_rs485 *rs485)
>  {
> +       struct serial_rs485 rs485_uapi;

> +       *((struct kserial_rs485 *)&rs485_uapi) = *rs485;

Ditto.

+ Blank line?

>         /* Return clean padding area to userspace */
> -       memset(rs485->padding0, 0, sizeof(rs485->padding0));
> -       memset(rs485->padding1, 0, sizeof(rs485->padding1));
> +       memset(rs485_uapi.padding0, 0, sizeof(rs485_uapi.padding0));
> +       memset(rs485_uapi.padding1, 0, sizeof(rs485_uapi.padding1));
>
> -       if (copy_to_user(rs485_user, rs485, sizeof(*rs485)))
> +       if (copy_to_user(rs485_user, &rs485_uapi, sizeof(rs485_uapi)))
>                 return -EFAULT;
>
>         return 0;

...

> +/* Compile-time asserts for kserial_rs485 and serial_rs485 equality (except padding) */

struct kserial_rs485
struct serial_rs485

(rationale: standard representation in text / comments and be a link
in case if this is converted to kernel doc)

...

> +/*
> + * Must match with serial_rs485 in include/uapi/linux/serial.h excluding the

Ditto.

> + * padding.
> + */
> +struct kserial_rs485 {
> +       __u32   flags;                  /* RS485 feature flags */
> +       __u32   delay_rts_before_send;  /* Delay before send (milliseconds) */
> +       __u32   delay_rts_after_send;   /* Delay after send (milliseconds) */
> +       struct {
> +               __u8    addr_recv;
> +               __u8    addr_dest;
> +       };

Btw, can't we convert them to kernel doc?

> +};

...

> + * There's kernel counterpart kserial_rs485 of this struct without padding.

struct kserial_rs485

-- 
With Best Regards,
Andy Shevchenko