Am 04.12.2017 um 21:18 schrieb Dan Carpenter:
On Mon, Dec 04, 2017 at 08:59:35PM +0200, Marcus Wolf wrote:

Am 04.12.2017 um 12:33 schrieb Dan Carpenter:
On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:
diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 34ff0d4807bd..bcfe29840889 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -63,7 +63,7 @@ struct pi433_tx_cfg {
        __u16                   bit_rate;
        __u32                   dev_frequency;
        enum modulation         modulation;
-       enum modShaping         modShaping;
+       enum mod_shaping        mod_shaping;

I looked at how mod_shaping is set and the only place is in the ioctl:

     789          case PI433_IOC_WR_TX_CFG:
     790                  if (copy_from_user(&instance->tx_cfg, argp,
     791                                          sizeof(struct pi433_tx_cfg)))
     792                          return -EFAULT;
     793                  break;

We just write over the whole config.  Including important things like
rx_cfg.fixed_message_length.  There is no locking so when we do things
like:

     385          /* fixed or unlimited length? */
     386          if (dev->rx_cfg.fixed_message_length != 0)
     387          {
     388                  if (dev->rx_cfg.fixed_message_length > 
dev->rx_buffer_size)
                              
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
check

     389                  {
     390                          retval = -1;
     391                          goto abort;
     392                  }
     393                  bytes_total = dev->rx_cfg.fixed_message_length;
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
set this in the ioctl after the check but before this line and it looks
like a security problem.

     394                  dev_dbg(dev->dev,"rx: msg len set to %d by fixed 
length", bytes_total);
     395          }

Anyway, I guess this patch is fine.

regards,
dan carpenter


Hi Dan,

you are mixing rx and tx. The part from IOCTL is copied from the tx-part,
the lower part is dealing with rx.

With rx there should be no problem, since IOCTL is blocked, as long as an rx
operation is going on.

With tx, I also expect no problems, since instance->tx_cfg is never used to
configure the rf69. Everytime, you pass in new data via write() a copy of
tx_cfg is made. Transmission is done, using the copy of the tx_cfg, never by
using instance->tx_cfg.

But maybe I didn't got your point and misunderstand your intention.


No.  You're right.  I mixed up rx and tx.  But the ioctl interface still
seems really horrible.  We generally frown on adding new ioctls at all,
but in this case to just write over the whole struct with no locking
seems really bad.

regards,
dan carpenter


In principle, you are right. But that's even a more macroscopic problem.

If someone sets a config, he needs for a telegram, and someone else comes in with another config, before the first one could fire his write,
shit will happen.
Same on RX-side: First one sets his config for receiving and will not get, what he wants, if someone else sets an other config, before he can fire his read.

If noone changes the config, until read() or write() was called, we are out of danger - even concerning the risk you mentioned.

An option to avoid that, could be, that every write and read transaction needs to pass in a complete config struct. There were reasons, not to do so, but we could think of implementing it that was.

Are there other options for configuration, despite IOCTL?

Cheers,

Marcus
Cheers,

Marcus

Reply via email to