Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Thu, 14 Apr 2022 18:49:50 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Wang Cheng <wanngchenng@gmail.com>
Cc:     Larry.Finger@lwfinger.net, florian.c.schilhabel@googlemail.com,
        gregkh@linuxfoundation.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac"
Message-ID: <20220414154949.GM3293@kadam>
References: <20220414141223.qwiznrwgjyywngfg@ppc.localdomain>
 <20220414154215.GL3293@kadam>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220414154215.GL3293@kadam>
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?pYweu9A7RQeM0l1CX/rFBl0kLaObr/uNIHMnPXLxPGK4SY8fb/d08Xpp7reT?=
 =?us-ascii?Q?e5DjJpQSf17EJMWLpsa2Lqajea3Rfmjac8qFU5cNZrs23118mX5upXq77dMX?=
 =?us-ascii?Q?8XsOJAsqKMxmnUXKqjw3ZlpUpoPN/x7mgT+rpPeRSQoTGzXCRF46GYDAcjwu?=
 =?us-ascii?Q?nkfOfR35TKv9rvvLvFgiRqHnY40FofsYYm0moMxbl8pPWK82CZqiWkal9nuw?=
 =?us-ascii?Q?nMt3nkRUiBVGvJV+xFVqdVv1uT9SLBqobXqp3cDdOqS+zv4s1VBDwefAYKEP?=
 =?us-ascii?Q?WeSDkzRrWU+GtUlt3DwDzFeLyjH6PBgAaJ8IIKn1fuFZ0fbCCesfX7bdwxJo?=
 =?us-ascii?Q?2GDRnlrFXJG+fd3Ux57umVADASkNk4QYaPD+guoQ5stTmXGa/MCKBBXpIww8?=
 =?us-ascii?Q?WTd3rbVTpefyOzcsFOHmVng/oqaxAru9sW4l0FaC1WUiSRi6LE07Y+xt3Mko?=
 =?us-ascii?Q?SEi7TgQJLcRYWpg490at1sVRYn9CX8LGWt7ThorgQVEi5Bs6VqmeWYmTS3IA?=
 =?us-ascii?Q?n9u9FlDVAuiN7QNuM2YQLv3PPGANcfaQ97Ja8NXTG5ZXzgUo5xsdSfazfO0i?=
 =?us-ascii?Q?n3ednK6++SE9WFIMV+qKtoNuPHNNNYMVFU6446Wbfejd9wzsI9WNIw5AExcg?=
 =?us-ascii?Q?u9FT4n016ev8TfMHMamUX1UJw2d5FCvNnwaRGw4hbwaK8cgP7TABooaG3buj?=
 =?us-ascii?Q?bSSk7u14sSYDx4lIMeur1z7sL26x8/gGkD072Tj/njJbfkXw0II0qDFsfWBz?=
 =?us-ascii?Q?o7O4cp9hIzEZfRkTnpWQAjY/cQl3s5308wh/vWY32zZm/V+sgaMynKhWQfIL?=
 =?us-ascii?Q?FvXEOEkSJqbIB2IICOG31b/AgCt1z9zDOqAkLmtPfOZsaiNSsAe/r+I00KKr?=
 =?us-ascii?Q?dbUL8fJv5qZwcUsstDt5DKVhN2ZsvNnmAL9BjWiDb/v6uZxiNOlAMcHLdXE+?=
 =?us-ascii?Q?9xbo95gsJq98SATlPFtPtAGEeIrDtO5VgaxXHMwIKyPnBm4gJVY+ZAV6NXTk?=
 =?us-ascii?Q?i8PQq3ZkDimiv2tM+BDVcFTMRePMjN3NBWRlZwEXQTiHNb3Fi2QqW1bLlczR?=
 =?us-ascii?Q?U2A5ROdKeT3WqFzceJetzNzFimme4Qb7b5blB3231U0mWchuIKf7aDmhNMcX?=
 =?us-ascii?Q?AvyGAOA6K3cVV8bAOo9E31M0sjjyAK4oTrwtGNGVN2TnBbUzdmWoFoI17LCs?=
 =?us-ascii?Q?CyO9B65TPMUxT3nWL0ZJjTu4aN30bwyff0TnVLjVChXphrWeItCaGiI51Qwr?=
 =?us-ascii?Q?gzzTs8KL9fljQPx8PynrGsAt8UUVklAseWo8xJeEumsoB7rbczX7lCvu58oi?=
 =?us-ascii?Q?8hDWr9ulshMoheWqllDGExap5asAgnMnY8WD0fFDuukLG4/rOpM2vxHo7Exb?=
 =?us-ascii?Q?5//DEjjHYAXvP+Yb+oSNrg4vSBOH2LnkY/IqUJtepIPceg/CtFlVnMYcvsGH?=
 =?us-ascii?Q?c5ryxzXi7W4WMAgug/kw0/aZQmlxtbhwox4QrYwyDe4pSyt6IJMh/SU3d1K8?=
 =?us-ascii?Q?WouT8F84BQzPpulIn16BJ2TSj+D0RdHXFylOMb5egU6OXb94oMd2HeJ25gyi?=
 =?us-ascii?Q?y93nJGFdENeHPQaXEA5NvDPChVjZR+l9luUTKmsgJfwzk8t3G5EL20X6evCW?=
 =?us-ascii?Q?bk/jX4OEQOjAmtFYYgcs5joR1EvfhrvCARU+6xo2dRHir9duC7qmzUrMRUIw?=
 =?us-ascii?Q?uKqrQT4UkHJ42hnt1u+4dZkq2l8EGvHraEh44iZawv1xKkblGmIdHRycVZkc?=
 =?us-ascii?Q?K50rKr6hTcs2v6VajNugPSP6jIQrtyA=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 93ba77a5-be32-49ed-c2c9-08da1e2e73d6
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Apr 2022 15:50:09.1973
 (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: rdnS4D24kjHMkjL/JXowiHab0hnhcFrqM1cyThh3gCiCI6/zvPtWZDpkIJ7jNlSruqHdQT2+2CFp/Y2YfcjwVH0RmPwMP4pC/ZUG7L5owYw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4472
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-14_04:2022-04-14,2022-04-14 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 adultscore=0
 phishscore=0 mlxscore=0 suspectscore=0 malwarescore=0 mlxlogscore=826
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2204140084
X-Proofpoint-ORIG-GUID: qxAwvspfzjaGkQRFTZc4GoRor-0aheJx
X-Proofpoint-GUID: qxAwvspfzjaGkQRFTZc4GoRor-0aheJx
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 Thu, Apr 14, 2022 at 06:42:15PM +0300, Dan Carpenter wrote:
> > diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c
> > index f984a5ab2c6f..e321ca4453ca 100644
> > --- a/drivers/staging/rtl8712/usb_ops_linux.c
> > +++ b/drivers/staging/rtl8712/usb_ops_linux.c
> > @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value,
> >  	}
> >  	status = usb_control_msg(udev, pipe, request, reqtype, value, index,
> >  				 pIo_buf, len, 500);
> > -	if (status > 0) {  /* Success this control transfer. */
> > -		if (requesttype == 0x01) {
> > -			/* For Control read transfer, we have to copy the read
> > -			 * data from pIo_buf to pdata.
> > -			 */
> > -			memcpy(pdata, pIo_buf,  status);
> > +	/* For Control read transfer, copy the read data from pIo_buf to pdata
> > +	 * when control transfer success; otherwise init *pdata with 0.
> > +	 */
> > +	if (requesttype == 0x01) {
> > +		if (status > 0)
> > +			memcpy(pdata, pIo_buf, status);
> > +		else
> > +			*(u32 *)pdata = 0;
> >  		}
> 
> This isn't really correct.  In many cases status is "len" is less than 4.
> I'm slightly surprised that nothing complains about that as an
> uninitialized access.  But then another problem is that "status" can be
> less than "len".
> 
> A better fix instead of setting pdata to zero would be to add error
> checking in the callers and then change this code to use
> usb_control_msg_send/recv().  Probably just initialize "data" in the
> callers as well.

A different, less good option would be to, still add error handling in
the callers, but do:

	status = usb_control_msg();
	if (status < 0)
		goto free;
	if (status != len) {
		status = -EREMOTEIO;
		goto free;
	}
	if (requesttype == 0x01)
		memcpy(pdata, pIo_buf, status);

	status = 0;
free:
	kfree(palloc_buf);
	return status;

regards,
dan carpenter