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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20220118135902.GH1951@kadam>
Date:   Tue, 18 Jan 2022 16:59:02 +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 Mon, Jan 17, 2022 at 07:02:20PM +1300, Paulo Miguel Almeida wrote:
> Max bit rate is dependent on which modulation is used. Previous
> validation routine only took into consideration min bit rate which can
> lead a misconfiguration of the rf69 chip causing the packets not to be
> sent/read.
> 
> This patch enhances that input check in set_bit_rate to account for
> modulation values and their respective max bit rate
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@...il.com>
> ---
> Meta-comments:
> 
> In the patchset v1 I kept bit_rate argument's original type as I thought that
> changing it to accomodate values as high as 300kbps couldn't be part of this
> patchset and therefore it should be a separate patchset. 
> 
> Given that kernel test bot compilation/test process 'complained' about the 
> argument's type, I decided to send the v2 patch that addresses the data type
> problem while I work on the patch that will change bit_rate type across
> tx_cfg and rx_cfg as this will require a bit more work.
> 
> Please let me know if anyone dislikes the approach and wants me to deal with it
> in a different way.

I always think that correct static checker warnings should be preserved
instead of papered over and silenced.  Someone went to a lot of work to
create that warning and then we're disabling it for being correct?  It
doesn't make sense.

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

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.  Each pi433_tx_cfg
struct member would become it's own file obviously.  We could pick sane
default values where ever possible instead of leaving it all up to the
user.

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.

(I say this with the complete confidence of someone who has never used
pi433 and is not really sure what it is).

regards,
dan carpenter