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:   Fri, 15 Apr 2022 13:02:23 +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: <20220415100223.GS3293@kadam>
References: <20220414141223.qwiznrwgjyywngfg@ppc.localdomain>
 <20220414154215.GL3293@kadam>
 <20220415094705.aibh3jr4wzhddmud@ppc.localdomain>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220415094705.aibh3jr4wzhddmud@ppc.localdomain>
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?YUtE4+KA7YG0EDJ8p9aSllx5tc4N3pWqu1PDebnUOu56BnxJ/JU7tJoMPCs/?=
 =?us-ascii?Q?f2oHn8ORDkrU9ra4yBVKmuVXWLK446W2zoJ5Pe4xu21X4nIaBCMC1/YBQ4/u?=
 =?us-ascii?Q?XDueQej+6wTtmmLlc44IVEnnTiVAsK0i28urbhuTYoLhso9Ss8BqZxp0bLzk?=
 =?us-ascii?Q?M0yDXKKwmfxadMlguikAd9nSPxqrEzUQW/V9yUyVCaFvsmK0m7zpsscY5za+?=
 =?us-ascii?Q?5Rr7mPv7XmTcX4hRH6zIgpVxIcMDfxi1k0YgCbSjUzudDf8x+zQlzvJdCJhM?=
 =?us-ascii?Q?vMQBEKioQ2FatqiC2bGdfXtIUG6C7daxcbrarT8HR0hEs92lJWDvsy9d3iFT?=
 =?us-ascii?Q?BxVdVwjmGK5hM9vLAgLQvGIf660DGKjRNvErYwT0+Eey8Fumk+hyAwgtDX+h?=
 =?us-ascii?Q?+PwnlAds1RpbSY3SIWm8NREFpPzlWWEi3sa1/lsQhjHsXq6iHvwFOES5leVt?=
 =?us-ascii?Q?6xjTrsXqJ4SrNcOambtrMrHR7WDow7fSebbK96fH/p+GaKP2A3tMjiL2/T16?=
 =?us-ascii?Q?HWEwqlRnAepVNg+1jKmKOeGkDS+6qzxmVGUEKuu3IO0CMFx2Po5poUqQdtTw?=
 =?us-ascii?Q?BdgQwXuOdUffNjW1GK/K2JrtlZtOt6T8IKcf8R/YA6CG/j7Pe5xm0FVwBPKe?=
 =?us-ascii?Q?vyN1OGyMzfcr2hcmJHQW7kZkwkLlGZrbMFrC5h4YaItOidegKpH6toowi9z2?=
 =?us-ascii?Q?dNLHcYaiwR42arNO/n7vC3yNRU5xQl1yd7EOjmcE+vt5VBMo28QlROndKZoU?=
 =?us-ascii?Q?4g+PArxymo2BPwoALE3vJpcqMMZOH2c23iD/byZb/iH3gzR/uMfyCKs8KoZJ?=
 =?us-ascii?Q?tr/rd64GDxc5TGY71+U9EKZxeVxscO+r56RjVDcE4D/4OgiV/xIi5IM6m4Oi?=
 =?us-ascii?Q?/fOLGiAkAVKPTbPdTMtYL9fd9gd5x482G1YzDEeNlOpULeVMF66tXEKMTwYr?=
 =?us-ascii?Q?belDec/bGL67mSCpZG6Sy+CWwFnWRZafpH/R5PzmBjz0hwHTSw7cOYshZhjk?=
 =?us-ascii?Q?F5ohCCIqWfMCUDkQq/+5TrpJNLLHO3GynHubPeSgw6URr61PAXBlGn1bqVsu?=
 =?us-ascii?Q?0dDPR2B9K9Btr+6S8UA2mLMbwlEteepBtgYiC7esvPt8wx0djAsJX6qKW7wU?=
 =?us-ascii?Q?hANwGujGZZV7c/gAzFECGIWAq/bCvBh4qhW4p4F+eorxXPJ6LG751g236mcC?=
 =?us-ascii?Q?wYvNVcvm8szNpJs9G+ePlPHDxkzyQbYVQEKMSLIzAxsvi8g5IH0bGpr9uA3h?=
 =?us-ascii?Q?BIgKhnATHrbc5yl3ERmkVtzAgfDI8bbejui8oU8e3eUOH7yOhn5itcMO7t2L?=
 =?us-ascii?Q?JAiClxlo8Ha+r49yRLlGMW3Xp+vfXNJrAko1AO7+XgiU6iSPzFHf5fHV6gtz?=
 =?us-ascii?Q?FrJxBkX3X3V1y9BMPn+FwOD+tzgw5qjVzQzcYOa/xAvbBw0i1CdGu0jJmgn7?=
 =?us-ascii?Q?nbQv04DhxFmgvM+Ud8UvRqFL4uQ6TikSbmjet6U0f2cToCw4fhCLmMSmNKYI?=
 =?us-ascii?Q?dT6fYvdLGE6AuDvMw53an8jPyrs4gStPbJPKIzt/Lay75oN3HdKnYxt7TkIk?=
 =?us-ascii?Q?Nhq783ZeSgskr43Di1hsDgUIXsttehDY3R+NxT7/DZP1EdzT4ctJ6OWNqWQh?=
 =?us-ascii?Q?b0sYaP4KTswOjgY2x8qS2xM+f7aphaF8C20noApWPSvcf3Jwefqz0FIPekSQ?=
 =?us-ascii?Q?GPWaVf510Bm6DMKTqAF5yGk27oPOFLvBYdU7696Cff32iIF5hsPce2b4TIsa?=
 =?us-ascii?Q?wVIJfrUuhnjTAcxzqlPLMnCIPLP/mxY=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bf651488-8b25-4ee6-bf73-08da1ec70f81
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Apr 2022 10:02:33.5082
 (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: uGdh//x6SIopayFJdaVLT9cbFkpVLOfDXeEdoRINNkaOIBoCIrvyPvzfmMVrHRQjZ+cMG56QNh3SzR5zYRTiv91vOwXEupAY5dPhCYOJFgM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB5418
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-15_01:2022-04-14,2022-04-15 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxlogscore=890
 spamscore=0 suspectscore=0 malwarescore=0 adultscore=0 bulkscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204150059
X-Proofpoint-ORIG-GUID: axMkpAKGCDVu9ACDtxIWPV2HiGGYRrXC
X-Proofpoint-GUID: axMkpAKGCDVu9ACDtxIWPV2HiGGYRrXC
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 Fri, Apr 15, 2022 at 05:47:05PM +0800, Wang Cheng 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".
> 
> Sorry, I should explain it clearly. If I did right, watching "status"
> with gdb while running syzkaller reproducer, "status" returns from
> usb_control_msg() is -71. In which case, *pdata won't be touched in
> r8712_usbctrl_vendorreq(). As a result, "data" in
> usb_read8()/usb_read16()/usb_read32() will be returned without
> initialization. I think that is why kmsan reports:
> Local variable data created at:
>  usb_read8+0x5d/0x130 drivers/staging/rtl8712/usb_ops.c:33
>  r8712_read8+0xa5/0xd0 drivers/staging/rtl8712/rtl8712_io.c:29
> 
> > 
> > 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.
> 
> I tried something similar which also works fine, but I think this patch
> does't fix it at root.
> https://syzkaller.appspot.com/text?tag=Patch&x=15be2970f00000

Oh...  I meant initialize data on the success path as well.

	__le32 data = 0;

You're saying that kasan generates a warning for your updated patch?
Earlier I mentioned "I'm slightly surprised that nothing complains about
that as an uninitialized access." so I guess maybe it does complain?
Good.

regards,
dan carpenter