linux-kernel - RE: [PATCH 1/3] tty: n_gsm: add keep alive support

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB9PR10MB588101762F5AA07EE43F0658E0D19@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>
Date:   Wed, 1 Feb 2023 09:48:58 +0000
From:   "Starke, Daniel" <daniel.starke@...mens.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "jirislaby@...nel.org" <jirislaby@...nel.org>,
        "ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/3] tty: n_gsm: add keep alive support

> > > > index cb8693b39cb7..b64360aca1f9 100644
> > > > --- a/include/uapi/linux/gsmmux.h
> > > > +++ b/include/uapi/linux/gsmmux.h
> > > > @@ -19,7 +19,8 @@ struct gsm_config
> > > >  	unsigned int mtu;
> > > >  	unsigned int k;
> > > >  	unsigned int i;
> > > > -	unsigned int unused[8];		/* Padding for expansion without
> > > > +	unsigned int keep_alive;
> > > > +	unsigned int unused[7];		/* Padding for expansion without
> > > 
> > > "unsigned int" is not really a valid uapi variable type.
> > > 
> > > Shouldn't this be __u32 instead?
> > 
> > I know but changing it to a fixed size data type may break compatibility
> > as this may change the overall size of the structure.
> 
> Will it?  It shouldn't that's why using the correct data types is
> essencial.

Well, unsigned int is defined to be at least 16 bit. Using __u32 will break
systems where this is true. I am not sure if the Linux kernel targets any
system which defines unsigned int with 16 bit. But sure, I can change it to
__u32.

> > This is why I
> > took a field out of the "unused" array for the "keep_alive" parameter.
> > A value of zero disables keep-alive polling.
> > 
> > > Should you document this field as to what the value is and the units as
> > > you are creating a new user/kernel api here.
> > 
> > I will add a comment here. Comments for the other fields remain subject to
> > another patch.
> > 
> > > And finally, "unused" here is being properly checked to be all 0, right?
> > > If not, then this change can't happen for obvious reasons :(
> > 
> > This was not the case until now. I assumed there was some coding guideline
> > that unused fields need to be initialized to zero. Obviously, checking it
> > prevents misuse here. I will add relevant checks for this.
> 
> If the value was not checked previously, then you can not use the field
> now, otherwise things will break, sorry.  Those are useless fields and
> should be marked as such :(

What is the way forward here? Should I introduce a complete new ioctl?
Or should I use a different size for this structure to break existing code
intentionally? Does this mean that we cannot extend this structure at all
in the future? I had planned another extension here to properly support
parameter negotiation.
In case we need to keep the structure as it is: Would a comment be
sufficient to mark this field accordingly?

Best regards,
Daniel Starke