netdev - Re: [PATCH net] mctp: serial: Fix starting value for frame check sequence

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]

Message-ID: <2eecaca2d1066d51d136a8d95b5cd2fd19e5e111.camel@gmail.com>
Date:   Fri, 16 Dec 2022 08:41:30 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Jeremy Kerr <jk@...econstruct.com.au>, netdev@...r.kernel.org
Cc:     Matt Johnston <matt@...econstruct.com.au>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Harsh Tyagi <harshtya@...gle.com>
Subject: Re: [PATCH net] mctp: serial: Fix starting value for frame check
 sequence

On Fri, 2022-12-16 at 11:44 +0800, Jeremy Kerr wrote:
> RFC1662 defines the start state for the crc16 FCS to be 0xffff, but
> we're currently starting at zero.
> 
> This change uses the correct start state. We're only early in the
> adoption for the serial binding, so there aren't yet any other users to
> interface to.
> 
> Fixes: a0c2ccd9b5ad ("mctp: Add MCTP-over-serial transport binding")
> 
> Reported-by: Harsh Tyagi <harshtya@...gle.com>
> Tested-by: Harsh Tyagi <harshtya@...gle.com>
> Signed-off-by: Jeremy Kerr <jk@...econstruct.com.au>
> ---
>  drivers/net/mctp/mctp-serial.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
> index 7cd103fd34ef..9f9eaf896047 100644
> --- a/drivers/net/mctp/mctp-serial.c
> +++ b/drivers/net/mctp/mctp-serial.c
> @@ -35,6 +35,8 @@
>  #define BYTE_FRAME		0x7e
>  #define BYTE_ESC		0x7d
>  
> +#define FCS_INIT		0xffff
> +
>  static DEFINE_IDA(mctp_serial_ida);
>  
>  enum mctp_serial_state {
> @@ -123,7 +125,7 @@ static void mctp_serial_tx_work(struct work_struct *work)
>  		buf[2] = dev->txlen;
>  
>  		if (!dev->txpos)
> -			dev->txfcs = crc_ccitt(0, buf + 1, 2);
> +			dev->txfcs = crc_ccitt(FCS_INIT, buf + 1, 2);
>  
>  		txlen = write_chunk(dev, buf + dev->txpos, 3 - dev->txpos);
>  		if (txlen <= 0) {
> @@ -303,7 +305,7 @@ static void mctp_serial_push_header(struct mctp_serial *dev, unsigned char c)
>  	case 1:
>  		if (c == MCTP_SERIAL_VERSION) {
>  			dev->rxpos++;
> -			dev->rxfcs = crc_ccitt_byte(0, c);
> +			dev->rxfcs = crc_ccitt_byte(FCS_INIT, c);
>  		} else {
>  			dev->rxstate = STATE_ERR;
>  		}

Since the starting value isn't unique would it possibly be worthwhile
to look at adding a define to include/linux/crc-ccitt.h to be used to
handle the cases where the initial value is 0xffff? I notice there
seems to only be two starting values 0 and 0xffff for all callers so it
might make sense to centralize it in one place.

Otherwise the code itself looks good.

Reviewed-by: Alexander Duyck <alexanderduyck@...com>