Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 4 Apr 2022 17:35:31 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc:     Charlie Sands <sandsch@northvilleschools.net>,
        gregkh@linuxfoundation.org, Larry.Finger@lwfinger.net,
        phil@philpotter.co.uk, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org, paskripkin@gmail.com
Subject: Re: [PATCH V2] Fix unsafe memory access by memcmp
Message-ID: <20220404143531.GE3293@kadam>
References: <YkpdVyuQguGzPSNE@sckzor-linux.localdomain>
 <4729027.31r3eYUQgx@leap>
 <20220404120332.GY3293@kadam>
 <1890716.PYKUYFuaPT@leap>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1890716.PYKUYFuaPT@leap>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?swfSJEUo0Q5mJgSGDihBgNYN42GgZn8h0KsXRMj/5PnSZKayXgQy5GVbK/g6?=
 =?us-ascii?Q?nVrppriWN05phBNof0L0pym1cfDcto8IWDtQg7Oxd3oOcCLmOuivxGkAzh+a?=
 =?us-ascii?Q?D7AZHEAgEMvzmD+rCDbWGAdMo92zF7az8/VZfS0S7PmvlD+twbkTUJdBEUJQ?=
 =?us-ascii?Q?cPAhMjtDHg6i7SFoAhkVKT+Xt6VY4Ob4IjQ6daZWBvnE07YC+fIKnR6XAK3C?=
 =?us-ascii?Q?hcItaAdzyUtVrDlL54rk/VagrVXs7LZ+vPT2ttUwwcF28Fo0eqU0FAh2TlRj?=
 =?us-ascii?Q?lu3LsgcIKJoaIZAOmzz+oINf+KByHipthctaPaYFWnF3qzrGbv+i+BhlxhzJ?=
 =?us-ascii?Q?rlsibcLrnY9jESydd/egs6K9vYHVO/oWvPiO2QZcsMX23vkHlfx43rMb7Mbv?=
 =?us-ascii?Q?OERJqbuUGZAZ7n8U8Gq2pVbejuhX6i48RS7N0i53nQV/EwsVAMCo0BGJ7F7V?=
 =?us-ascii?Q?axXm3XMTz/daBIqYZox3cKc3cKnh3Ar2HyjfOOJR5+SwFHHGoXPhvgrMZYLO?=
 =?us-ascii?Q?PAPgOjvQrLzYluzBGXHAPVYjRFMAQ6LAJiRl8w3uNi2IzC9wsVlZpH7FUOyT?=
 =?us-ascii?Q?mDJ996i8STryaDhdPQnaPI/0zOmQ9CvZiWmKrZcIRI8AVfDgQtHsfAHwJCuU?=
 =?us-ascii?Q?7hP46CSiDAxM6zfIk/0J2g6Q+OaRCo/ytjI4VFwRow5YADoUrGzrmzJYUmMK?=
 =?us-ascii?Q?fTL5onAH1SeEj3KmU22FTBcgZNSyl0tEagSGZ5pOokmQ7j7yg+agJPtTvRaN?=
 =?us-ascii?Q?m1lr0s0CBS7i0SOV3iB3t9/dg/tFc7eU91Xu6sbL4CrTmnPpQuWmvgrdT/sb?=
 =?us-ascii?Q?AHTyApwAq9vtdjftEgeNCH5MAlFZ/XT+bM7t/lLBwWoWxBdir027y1SOEEic?=
 =?us-ascii?Q?4jqf2be5U+5HGQ44IEnYYiyFfX++AZOa7G+qhbzq20T7t/mEM0dJRYr3TPC4?=
 =?us-ascii?Q?RDRcPbbGe+yZfd4Xg27BEkerQh1R0dP6HZbGBSr4YjjYs3ztZIPUFKjdurfn?=
 =?us-ascii?Q?fgu2u2P5BrVcyChXKoZwaCJsEKsst22PZKmSv7jmxvfiAerKcGHMZ1H1FZFL?=
 =?us-ascii?Q?6Cw8bWoqmygF6KJ7QCME2qixkM/beaIfW4I/K5y4/BtbsoQ2UODsV16uYE3S?=
 =?us-ascii?Q?CPCh40rp5O8bDQvMT/4u/OfSnOzk10MlLHbQUZWAkAEIilhxveBhY1z8KwMp?=
 =?us-ascii?Q?FT+axO01PPOtYEEaGwRLXDrmFHiEI6GEQqcQxoUUr0F9S/N057lqIGCyk+VG?=
 =?us-ascii?Q?1NgZUNifUspqLvlZZH2XEd7Xmf9b2SsJ0m46nf/kew8LIsOBlHBEz1UsoEMa?=
 =?us-ascii?Q?JXMVOtHPJPOy6+0Nk8p+IMEq2gfWQc1sm+5PUe64MmAOxFC4GIt3VpPSnpGd?=
 =?us-ascii?Q?znv3xBLhN+UjunfoBEiLbgYZp0ANULvywlvG3OKAfu2215bvrDNy32K1xaJ9?=
 =?us-ascii?Q?DNgwfp9FA24ac7QyXI/IyZ6+hUeZ/lD76FV7xGXk3/t5ZA1jKOzE8PKoR3Dg?=
 =?us-ascii?Q?AVoZ6AXhUzYOjrAaBKrZFjYG2H4Ys+3bSJHxphRfvuSWO61EcEuisLVi9Qke?=
 =?us-ascii?Q?xBUn5551Wx+4dqYhNjgcIPk/bVrMCy7+tL2kqswHaekmUw8YDfFwQLZEb9Q3?=
 =?us-ascii?Q?8zcmq4tX0PiIp4aJChTXlBtzx2I8ZYddjHlEJUSP3JGVfZf4ZDVZnVm2lFPy?=
 =?us-ascii?Q?MGONQ5osEYEFfIzXppHnuhFX1+pPU/91RBOyT4hD34HCgNo3BPd40Iqv9QXt?=
 =?us-ascii?Q?VAPXIWMnAgeTK/pjSFpj2dQNajkazTA=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c34f9af3-7b5a-4318-b778-08da16486c5c
X-MS-Exchange-CrossTenant-AuthSource: CY4PR1001MB2358.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Apr 2022 14:35:53.9068
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rwJDt/9T+C5WqxXfX5MofJ4AMsOJBQN7MvgFYFekLiN8NeX6psJe9zhZJ/gw1LTQ9i9sqmiyqXSDTuN9rcEPuJiCRDYitaliL5RTxhl/m20=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR10MB3250
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.425,18.0.850
 definitions=2022-04-04_05:2022-03-30,2022-04-04 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 bulkscore=0
 suspectscore=0 adultscore=0 phishscore=0 mlxscore=0 spamscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204040082
X-Proofpoint-ORIG-GUID: M0ERy4Bi8M5h8xz_n8aJkJWfE4KKaOQY
X-Proofpoint-GUID: M0ERy4Bi8M5h8xz_n8aJkJWfE4KKaOQY
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Apr 04, 2022 at 04:29:48PM +0200, Fabio M. De Francesco wrote:
> On luned? 4 aprile 2022 14:03:32 CEST Dan Carpenter wrote:
> > On Mon, Apr 04, 2022 at 01:25:37PM +0200, Fabio M. De Francesco wrote:
> > > On luned? 4 aprile 2022 12:50:41 CEST Dan Carpenter wrote:
> > > > On Sun, Apr 03, 2022 at 10:52:07PM -0400, Charlie Sands wrote:
> > > > > This patch fixes sparse warnings about the memcmp function unsafely
> > > > > accessing userspace memory without first copying it to kernel space.
> > > > > 
> > > > > Signed-off-by: Charlie Sands <sandsch@northvilleschools.net>
> > > > > ---
> > > > > 
> > > > > V2: Fixed checkpatch.pl warning and changed variable name as suggested
> > > > > by Greg K. H. and improved error checking on the "copy_from_user" function as
> > > > > suggested by Pavel Skripkin.
> > > > > 
> > > > >  drivers/staging/r8188eu/os_dep/ioctl_linux.c | 21 ++++++++++++--------
> > > > >  1 file changed, 13 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > > > > index 7df213856d66..4b4eec2bde96 100644
> > > > > --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > > > > +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > > > > @@ -3233,23 +3233,28 @@ static int rtw_p2p_get(struct net_device *dev,
> > > > >  			       struct iw_request_info *info,
> > > > >  			       union iwreq_data *wrqu, char *extra)
> > > > >  {
> > > > > -	if (!memcmp(wrqu->data.pointer, "status", 6)) {
> > > > > +	char wrqu_data[9];
> > > > > +
> > > > > +	if (copy_from_user(wrqu_data, wrqu->data.pointer, 9) != 0)
> > > > > +		return 0;
> > > > 
> > > > return -EFAULT;  We can't assume that that user wants to copy 9 bytes
> > > > especially when they're passing a 4 character + NUL string.
> > > > 
> > > > This is a custom ioctl.  Called from ioctl_private_iw_point().
> > > > 
> > > > I think if you try to dereference a user pointer like this then it will
> > > > cause a crash, right?  So that means no one has ever tested or used this
> > > > code and we hopefully we can just delete it?
> > > 
> > > After a quick look, I'm pretty confident that we can also delete 
> > > rtw_p2p_get2() and rtw_p2p_set() unless I'm overlooking something.
> > 
> > What are the problems with rtw_p2p_get2() and rtw_p2p_set()?
> > 
> > regards,
> > dan carpenter
> > 
> Is it safe to access user space pointers without using proper helpers? 

No.

> In those cases I mean: is it safe without using copy_from_user()?

Correct.  You need to use copy_from_user().

> 
> As I said, perhaps I'm overlooking something. However my conclusions 
> follow by your own argument.
> 
> If I understand what you wrote, you asked to delete rtw_p2p_get()
> because it looks like nobody "has ever tested or used this code".
> 
> rtw_p2p_get2() and rtw_p2p_set() use the same pattern of rtw_p2p_get()
> when they access user space without using the proper helpers.

Those functions use "extra" which is a kernel pointer.  Which user
pointer do they use?  Sparse doesn't detect it.

regards,
dan carpenter