linux-kernel - Re: [PATCH 1/3] tty: n_gsm: introduce macro for minimal unit size

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: <fe537e8-7b2-61e2-767d-787b923c7456@linux.intel.com>
Date:   Fri, 21 Oct 2022 12:40:17 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     "D. Starke" <daniel.starke@...mens.com>
cc:     linux-serial <linux-serial@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] tty: n_gsm: introduce macro for minimal unit size

On Fri, 21 Oct 2022, D. Starke wrote:

> From: Daniel Starke <daniel.starke@...mens.com>
> 
> n_gsm has a minimal protocol overhead of 7 bytes. The current code already
> checks whether the configured MRU/MTU size is at least one byte more than
> this.
> 
> Introduce the macro MIN_UNIT_SIZE to make this value more obvious.
> 
> Signed-off-by: Daniel Starke <daniel.starke@...mens.com>
> ---
>  drivers/tty/n_gsm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 5e516f5cac5a..8e039f2a0427 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -89,6 +89,7 @@ module_param(debug, int, 0600);
>   */
>  #define MAX_MRU 1500
>  #define MAX_MTU 1500
> +#define MIN_UNIT_SIZE 8
>  /* SOF, ADDR, CTRL, LEN1, LEN2, ..., FCS, EOF */
>  #define PROT_OVERHEAD 7

Why not call it just MIN_MTU?

I know you check it also against MRU but that seems so minor problem to me 
it's not worth even noting because of course MRU is related MTU, it's just 
the other end of the pipe. To be honest, I don't understand why MAX_MRU is 
even defined there when is just the same as MAX_MTU :-).

You could do this btw:

#define MIN_MTU	(PROT_OVERHEAD + 1)

To make it even more obvious where it comes from (matching to what you 
describe in your commit message).

The change looks otherwise ok.

-- 
 i.


>  #define	GSM_NET_TX_TIMEOUT (HZ*10)
> @@ -2712,7 +2713,9 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>  	if ((c->adaption != 1 && c->adaption != 2) || c->k)
>  		return -EOPNOTSUPP;
>  	/* Check the MRU/MTU range looks sane */
> -	if (c->mru > MAX_MRU || c->mtu > MAX_MTU || c->mru < 8 || c->mtu < 8)
> +	if (c->mru < MIN_UNIT_SIZE || c->mtu < MIN_UNIT_SIZE)
> +		return -EINVAL;
> +	if (c->mru > MAX_MRU || c->mtu > MAX_MTU)
>  		return -EINVAL;
>  	if (c->n2 > 255)
>  		return -EINVAL;
> @@ -3296,7 +3299,7 @@ static int gsm_create_network(struct gsm_dlci *dlci, struct gsm_netconfig *nc)
>  		return -ENOMEM;
>  	}
>  	net->mtu = dlci->gsm->mtu;
> -	net->min_mtu = 8;
> +	net->min_mtu = MIN_UNIT_SIZE;
>  	net->max_mtu = dlci->gsm->mtu;
>  	mux_net = netdev_priv(net);
>  	mux_net->dlci = dlci;
>