Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

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

Hi Dan,

unexpectetly I was into the driver code today, because a customer asked 
for an enhancment. In doing so, I also had a look at the points we 
discussed above.

Since both - the tx_cfg and the rx_cfg buffer belong to the instance, in 
order to get into trouble, you need to use the same file descriptor. If 
an other app is changing its config, it doesn't touch the current app.

So for RX: If a programm has called read(), it won't be able to 
succesfully call ioctl any more, because it is blocked:

        case PI433_IOC_WR_RX_CFG:
                mutex_lock(&device->rx_lock);

                /* during pendig read request, change of config not 
allowed */
                if (device->rx_active) {
                        mutex_unlock(&device->rx_lock);
                        return -EAGAIN;
                }

For TX in fact there is a little risk:
If a programm is using two tasks and passes the descriptor to both 
tasks, one is using the ioctl() and one is using write() and they are 
not synchronised, it might happen, that the ioctl is in the middle of 
the update the tx_cfg, while the write() is in the middle of copying the 
tx_cfg to the kernel fifo.
On one hand, that might be an "open point" at the driver, on the other 
hand no one will do such a programm architecture. Even if the driver 
will prevent a broken tx_cfg by mutex, the programm will never know, 
what it gets, if it issues ioctl() and write() unsynchronised from 
different tasks.
For fixing the driver, it might help to lock the write to the tx_cfg in 
ioctl() with the tx_fifo_lock, since write() is only copying the tx_cfg 
if it has the tx_fifo_lock.

I am not 100% sure. Maybe you (or someone else) want to crosscheck?

Regards,

Marcus