netdev - Re: [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl

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: <589EED15.8090303@bfs.de>
Date:   Sat, 11 Feb 2017 11:53:09 +0100
From:   walter harms <wharms@....de>
To:     Ralf Baechle <ralf@...ux-mips.org>
CC:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Thomas Osterried <thomas@...erried.de>,
        linux-hams@...r.kernel.org
Subject: Re: [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl



Am 11.02.2017 00:01, schrieb Ralf Baechle:
> When looking at Thomas' mkiss fix 7ba1b6890387 ("NET: mkiss: Fix panic")
> I noticed that the mkiss SIOCSIFENCAPS ioctl was also doing a slightly
> strange assignment 
> 
>                dev->hard_header_len = AX25_KISS_HEADER_LEN +
>                                       AX25_MAX_HEADER_LEN + 3;
> 
> AX25_MAX_HEADER_LEN already accounts for the KISS byte so adding
> AX25_KISS_HEADER_LEN is a double allocation nor does the "+ 3" seem to
> be necessary.  So this can be simplified to
> 
>                dev->hard_header_len = AX25_MAX_HEADER_LEN
> 
> which after the preceeding fix is a redundant assignment of what
> ax_setup has already assigned so delete the line.  The assignments
> to dev->addr_len and dev->type are similarly redundant.
> 
> The SIOCSIFENCAP argument was never checked for validity.  Check that
> it is 4 and return -EINVAL if not.  The magic constant 4 dates back to
> the days when KISS was handled by the SLIP driver where it had the
> symbol name SL_MODE_AX25.
> 
> Since however mkiss.c only supports a single encapsulation mode there
> is no point in storing it in struct mkiss so delete all that.
> 
> Note that while useless we can't delete the SIOCSIFENCAP ioctl as
> kissattach(8) is still using it and without mkiss issuing a
> SIOCSIFENCAP ioctl an older kernel that does not have Thomas' mkiss fix
> would still panic on attempt to transmit via mkiss.
> 
> 6pack was suffering from the same issue except there SIOCGIFENCAP was
> return 0 for the encapsulation while the spattach utility was passing 4
> for the mode, so the mode check added for 6pack is a bit more lenient
> allow the values 0 and 4 to be set.  That way we retain the option
> to set different encapsulation modes for future extensions.
> 
> Signed-off-by: Ralf Baechle <ralf@...ux-mips.org>
> 
>  drivers/net/hamradio/6pack.c | 10 ++++------
>  drivers/net/hamradio/mkiss.c | 10 ++++------
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
> index 470b3dc..d949b9f 100644
> --- a/drivers/net/hamradio/6pack.c
> +++ b/drivers/net/hamradio/6pack.c
> @@ -104,7 +104,6 @@ struct sixpack {
>  	int			buffsize;       /* Max buffers sizes */
>  
>  	unsigned long		flags;		/* Flag values/ mode etc */
> -	unsigned char		mode;		/* 6pack mode */
>  
>  	/* 6pack stuff */
>  	unsigned char		tx_delay;
> @@ -723,11 +722,10 @@ static int sixpack_ioctl(struct tty_struct *tty, struct file *file,
>  			break;
>  		}
>  
> -		sp->mode = tmp;
> -		dev->addr_len        = AX25_ADDR_LEN;
> -		dev->hard_header_len = AX25_KISS_HEADER_LEN +
> -		                       AX25_MAX_HEADER_LEN + 3;
> -		dev->type            = ARPHRD_AX25;
> +		if (tmp != 0 && tmp != 4) {
> +			err = -EINVAL;
> +			break;
> +		}
>  

What is about a comment like:
/*
The magic constant 4 dates back to the days when KISS was handled by the SLIP driver where it had the
 symbol name SL_MODE_AX25.
*/

just not to confuse future readers ..

re,
 wh


>  		err = 0;
>  		break;
> diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
> index 1dfe230..cdaf819 100644
> --- a/drivers/net/hamradio/mkiss.c
> +++ b/drivers/net/hamradio/mkiss.c
> @@ -71,7 +71,6 @@ struct mkiss {
>  #define AXF_KEEPTEST	3		/* Keepalive test flag		*/
>  #define AXF_OUTWAIT	4		/* is outpacket was flag	*/
>  
> -	int		mode;
>          int		crcmode;	/* MW: for FlexNet, SMACK etc.  */
>  	int		crcauto;	/* CRC auto mode */
>  
> @@ -841,11 +840,10 @@ static int mkiss_ioctl(struct tty_struct *tty, struct file *file,
>  			break;
>  		}
>  
> -		ax->mode = tmp;
> -		dev->addr_len        = AX25_ADDR_LEN;
> -		dev->hard_header_len = AX25_KISS_HEADER_LEN +
> -		                       AX25_MAX_HEADER_LEN + 3;
> -		dev->type            = ARPHRD_AX25;
> +		if (tmp != 4) {
> +			err = -EINVAL;
> +			break;
> +		}
>  
>  		err = 0;
>  		break;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hams" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html