Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752954AbdLDTSo (ORCPT ); Mon, 4 Dec 2017 14:18:44 -0500 Received: from userp2130.oracle.com ([156.151.31.86]:44047 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752538AbdLDTSj (ORCPT ); Mon, 4 Dec 2017 14:18:39 -0500 Date: Mon, 4 Dec 2017 22:18:19 +0300 From: Dan Carpenter To: Marcus Wolf Cc: Simon =?iso-8859-1?Q?Sandstr=F6m?= , devel@driverdev.osuosl.org, gregkh@linuxfoundation.org, linux@Wolf-Entwicklungen.de, linux-kernel@vger.kernel.org Subject: Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h Message-ID: <20171204191819.dmjrzw6ws457gzjo@mwanda> References: <20171203151726.16639-1-simon@nikanor.nu> <20171203151726.16639-7-simon@nikanor.nu> <20171204103356.cxmblmuvva3kjst6@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170609 (1.8.3) X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=8735 signatures=668637 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1712040272 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2967 Lines: 75 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