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

Message-Id: <20170211.213044.1550234417843321703.davem@davemloft.net>
Date:   Sat, 11 Feb 2017 21:30:44 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     wharms@....de
Cc:     ralf@...ux-mips.org, netdev@...r.kernel.org, thomas@...erried.de,
        linux-hams@...r.kernel.org
Subject: Re: [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl

From: walter harms <wharms@....de>
Date: Sat, 11 Feb 2017 11:53:09 +0100

> 
> 
> 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 ..

Agreed, every magic comment needs a define or a big comment.