linux-kernel - Re: [PATCH v2 3/3] staging: pi433: validate max bit

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]

Date:   Wed, 19 Jan 2022 08:34:11 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@...il.com>
Cc:     gregkh@...uxfoundation.org, realwakka@...il.com,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] staging: pi433: validate max bit_rate based on
 modulation used

On Wed, Jan 19, 2022 at 08:34:22AM +1300, Paulo Miguel Almeida wrote:
> On Tue, Jan 18, 2022 at 04:59:02PM +0300, Dan Carpenter wrote:
> > At the same time, this is a GCC warning so it will break the build.
> > 
> > Instead of applying this patch, I wish you would just make a note of it
> > in the drivers/staging/pi433/TODO file.  
> > 
> >     "Change (struct pi433_tx_cfg)->bit_rate to be a u32 so that we can
> >      support bit rates up to 300kbps per the spec."
> 
> thanks for taking the time to review this patchset.
> 
> It makes sense to me. So essentially for this version of the patchset I
> would only validate OOK max bit rate as it fits within the u16 boundary
> of the current implementation, right?
> 
> Something akin to this:
> 
> // check input value
> if (bit_rate < 1200 || (mod == OOK && bit_rate > 32768)) {
> 	dev_dbg(&spi->dev, "setBitRate: illegal input param");
> 	return -EINVAL;
> }

Actually, on further reflection, your patch is fine.  But please put a
note in the TODO that ->bit_rate needs to be changed to a u32.  Just
resend this patch series with that additional note in the TODO.

> 
> > 
> > But you're right that it's complicated to fix this because it's part of
> > the UAPI.  I think that the UAPI for pi433 is kind of garbage.  No one
> > like custom ioctls.  It would be better to use sysfs.  
> > ...
> > So my idea is that instead of modifying the custom ioctl then we can
> > just add a new sysfs file to set the bit_rate and all the other stuff.
> > Eventually we will delete the ioctl after all the users have updated to
> > the new userspace.
> > 
> 
> Using sysfs or even possibly configfs was one of the things I had in
> mind too. 
> 
> I started a similar discussion on how to change/remove ioctl:
> https://lore.kernel.org/kernelnewbies/YeVoAlv0ubKrmckV@kroah.com/T/#t 
> 
> The tldr;:
> 
> If there is a userspace tool that interfaces with char
> device and we can change it at the same time as the kernel code then we 
> can have a bit more flexibility on how to send/retrieve values to/from 
> the driver. If not then we would be required to keep the original ioctl
> compatibility even if we move to a different approach due to the
> impossibility of knowing whether or not someone is relying on that. :(
> 
> Everything leads me to believe that there isn't such userspace tool /
> lib so I emailed the original author just to be 100% sure that this is
> the case.

I don't know which is more appropriate sysfs or configfs.  I haven't
used configfs much and also I don't know much about pi433.

But I would encourage you to leave the ioctl as is.  There is no
advantage to the ioctl except backwards compatability.  This stuff can
all be done with sysfs.  It would be easier and more flexible going
forward.

regards,
dan carpenter