Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3cec4568-1a54-2ca9-57d6-ee6635d59a3b@oracle.com>
Date:   Wed, 13 Apr 2022 14:05:47 +0530
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.6.1
Subject: Re: [PATCH] cifs: potential buffer overflow in handling symlinks
Content-Language: en-US
To:     Steve French <smfrench@gmail.com>
Cc:     Dan Carpenter <dan.carpenter@oracle.com>,
        Steve French <sfrench@samba.org>,
        Stefan Metzmacher <metze@samba.org>,
        samba-technical <samba-technical@lists.samba.org>,
        LKML <linux-kernel@vger.kernel.org>,
        ronnie sahlberg <ronniesahlberg@gmail.com>
References: <20220412043823.57037-1-harshit.m.mogalapalli@oracle.com>
 <CAH2r5mvBH8wO9rS2UwCpgjbhcLp89-RLbr1wxK76Pj=OprKwOA@mail.gmail.com>
From:   Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
In-Reply-To: <CAH2r5mvBH8wO9rS2UwCpgjbhcLp89-RLbr1wxK76Pj=OprKwOA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R0dqUW5KTitBMm9ha29JZVZyYnJER2Z3Rmo2OHA5clFRTGg2WVA1cndybVcx?=
 =?utf-8?B?WTE2aStocWxKZUlJa1V5U1F0R2VOeExuWkdUWmJCNFdmRnNDZG5acTBZM1hI?=
 =?utf-8?B?RGxTZHJMSW5WUzlYMmVSaTJWdlNNU1hIS3dRQnZRTHlUckhhK1dYVGxmYnp2?=
 =?utf-8?B?dzVPWlBkUGZ0T0J1TnplTXY2dWpxMmdUcVlBTDRMbFRsOXZPUllqbnhUemxO?=
 =?utf-8?B?VVEvK2phWVVRUkcvOTNkU3Y2K1VRM0ZOazd2ekhzRW1LUWZ2TitSRWhQQVZ1?=
 =?utf-8?B?MENvaloxOXAwQm9kbUdqQ045bEtnUkZLdDBMRVB4TU9ST0JFUW51Z1hwdHUy?=
 =?utf-8?B?bTJrcE9iWDdKVjFsSDJrT0QzTkZQUHNWeC95ZVY5b05JRHNzWFBTSGRPTFhl?=
 =?utf-8?B?RDdRU0RSYXpQTElDYXpmZWJKR2R0cmFDcExvYnRkYTlOa1lTbmNuaE9XZkYr?=
 =?utf-8?B?NjJPUEJLYzZZeXpKdE14UG1BSWRUZ2Z6VGwzaXVWc1V2SW85RFF3Y0lNam5V?=
 =?utf-8?B?QXRMQTZEcDNiK2VSNmljckJCbmYyMk1raUNwZnRhL1dhbjlYUzU2VjI0L0o3?=
 =?utf-8?B?RDQ5cTM2WHZXR1ZwK1JQRnV4VGVyMWlFblNNSEJKeE11UG9uWXBnRTdpQUkr?=
 =?utf-8?B?Q1h6M3RVNW1lMDI4ZEV3YWFHZjY5emVMcW53bU9lVlNXakJtNmZOZ1k2L0E1?=
 =?utf-8?B?M0hkWUNod1pJbit0SnExMTVRSWdxVXBpcExRUGwva1pPZ2lsSEJ3Ym1EWThs?=
 =?utf-8?B?SnlObjlkUzZ2RnlZTUZLdnBwcWk4aFd0Ly9RdGFCeGp3VlR3UFVjVXdjWCtQ?=
 =?utf-8?B?S3YwVkcwWWp6VkVOY09palVaL0xjaDh3bGQwVmptNDRWK1I4ZC9zVHlRTFFm?=
 =?utf-8?B?Q3dkYjNodFRuSHc3dGhLRGY4emZSTHJ3NEtEQzY2TmVDRHRYQnhXTjFETXRJ?=
 =?utf-8?B?dC9FTC9MbFQxOFlpeHJDYXJoY2Fndk5VRzdZQ0NkV1ViYy9aeG1BUDFpN2N3?=
 =?utf-8?B?ajdzS3o0T21rei9zVG1rMUMwWWdsR21FLzc5NVdJK2RiTm1MYk1CVVhwMytm?=
 =?utf-8?B?VUw2WlIwM0VqRHNHeTF2ektueUpGcTNmbFRBOXA5SjJSbVZ1UDU2NEV0cUh2?=
 =?utf-8?B?eXFsN0RoRjA3NjBvb05PREpiNHFHRDQ2NHBKSFc4bit0clJ6Q0ZaaU92cHl4?=
 =?utf-8?B?MXhuTml2RytSdndHMTJubGxOZlc2emd0a3BFNUdDQU9lUVU5SENtcXlhbE8v?=
 =?utf-8?B?TC91QitObFpPb3gvMkRidkxIckVEOHBJd0pKOGxGVElmZHVtcUhVVzVwTnNP?=
 =?utf-8?B?S0c0OVBVYi83NnBheDVCSm1KWW0yOTZQRTkwWXMxMnNrQnVLcEk2ckVrQUdL?=
 =?utf-8?B?bm1YM0ZCcVlGb1lzbS9GWURxWk5hWElNaWUvR0UvLzcvNGRoL1pNa0I4aXFh?=
 =?utf-8?B?QVRhWElpOWVtaWNiQW1hWlBSWXB3Wnl4VjdvUXVrV3FFZFpFdEJtTTExenc3?=
 =?utf-8?B?ai93Q21DMEwzall3dTZZQk44ejM5ZllJRTFKTEt0R1lxMDBJS09laVhwOXVw?=
 =?utf-8?B?L0NWS3A5bUJSQis2Q0hYc1dMRFVUYStSMFplQnBZU3ZLZ1dFMllzaCtXZ1J5?=
 =?utf-8?B?WGdMdHhoTnJuT043bkl3Sm5kMU1yTm9zeS9yWFptKytIbUZQMHZmaUdmMnpu?=
 =?utf-8?B?RFF3YzV1UTJMQ1owRks3MzhzR2R2VGhzZDhodlVNY0tOK29MWGtXSGx4ZG0y?=
 =?utf-8?B?QUNoNFNTcFQxRDZGeVBVL2JRTEJqdWtJS2pzZmY3MUJLck5qbHhnZDZvVWMx?=
 =?utf-8?B?WWxxaFVtckJ0blpSb3hyVW1obVkvVk9jZVphWHZLRzVPMVd0WHdEcVBTb0U1?=
 =?utf-8?B?SmJEYXNPVllYYWNtOVNObVV1eHNFd01IZVhFc1hidkhlWG93K3VDUUV2SXlE?=
 =?utf-8?B?RFZpT0VmZFBIQ0loZGxUMkVqelFJaTJjcFdIMUZlQldJNHhDbFZMa2Jmb1dM?=
 =?utf-8?B?eHo2MTlaNUw4SVE3SmcxTGdhS05wTHg0S0tqWEtBaXR6R2hhbkJrcXovQVZH?=
 =?utf-8?B?S1FUdFZjUXZ4M1lpT0RBZS8wVG0xMEprVnlkbzB3M0wvaUNKNWxUVk1vMHd4?=
 =?utf-8?B?d2dzdjJZZkNCMjg2VUVNVEIvRHVKd3A0RlplTFRsYUcrUWhKVjk4d2tROGNG?=
 =?utf-8?B?YmUxdHBaakVnL051MDVQSjA1Nkp5ZFFpZmpTRkR0RXF1c3BLeW55aWRER2Rs?=
 =?utf-8?B?ZjIrWW9Cd0U2YzkrcG9reGVYQ28wSHJxcEFXc3NQVG4rdndwTWtHWHJ2OVJ5?=
 =?utf-8?B?L1pyYjRSb29iZHkyTWZTckFXb1dqc2RsYmxQSlRVS3kweTF4a1o0bTQ1Tkxi?=
 =?utf-8?Q?MqXhImCnknCrG9hO3pRXfHc739NJ6szmrOzk/?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0182f887-fa7e-4327-67e1-08da1d28a311
X-MS-Exchange-CrossTenant-AuthSource: MN2PR10MB4191.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Apr 2022 08:35:59.9164
 (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: CSTeaxHBjD6S+vWG6QnXEfHJlMN5T0JuIZA/LQy1tVrGMBq/C9PZf0p86sLsDEXpYBGfyObJP0fhZC0dlJ20LT5pC/AYt1VKd2cs5Mm9Db+SjWJMESpNMirrPHP487dd
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB5266
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-12_08:2022-04-11,2022-04-12 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxlogscore=999
 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-2204130047
X-Proofpoint-ORIG-GUID: SGEhhrPkx86fYMUJn3MIgxDFeNWacvgi
X-Proofpoint-GUID: SGEhhrPkx86fYMUJn3MIgxDFeNWacvgi
X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham 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

Hi Steve,
On 13/04/22 4:02 am, Steve French wrote:
> Wouldn't it be easier and clearer to do the compare vs the maximum len ie
> 
> if (link_len > CIFS_MF_SYMLINK_LINK_MAXLEN)
> 
Yes, thats cleaner. Will send a v2 patch.

Thanks,
Harshit
> instead of
> 
> if (link_len > buf_len - CIFS_MF_SYMLINK_LINK_OFFSET)
> 
> since buf_len is  CIFS_MF_SYMLINK_FILE_SIZE and looking at link.c line
> 26 and 27 this means we can use CIFS_MF_SYMLINK_LINK_OFFSET for the
> comparison:
> 
> #define CIFS_MF_SYMLINK_LINK_MAXLEN (1024)
> #define CIFS_MF_SYMLINK_FILE_SIZE \
>          (CIFS_MF_SYMLINK_LINK_OFFSET + CIFS_MF_SYMLINK_LINK_MAXLEN)
> 
> On Tue, Apr 12, 2022 at 1:01 AM Harshit Mogalapalli
> <harshit.m.mogalapalli@oracle.com> wrote:
>>
>> Smatch printed a warning:
>>          arch/x86/crypto/poly1305_glue.c:198 poly1305_update_arch() error:
>>          __memcpy() 'dctx->buf' too small (16 vs u32max)
>>
>> It's caused because Smatch marks 'link_len' as untrusted since it comes
>> from sscanf(). Add a check to ensure that 'link_len' is not larger than
>> the size of the 'link_str' buffer.
>>
>> Fixes: c69c1b6eaea1 ("cifs: implement CIFSParseMFSymlink()")
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>> ---
>>   fs/cifs/link.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/cifs/link.c b/fs/cifs/link.c
>> index 852e54e..ebfedae 100644
>> --- a/fs/cifs/link.c
>> +++ b/fs/cifs/link.c
>> @@ -85,6 +85,9 @@
>>          if (rc != 1)
>>                  return -EINVAL;
>>
>> +       if (link_len > buf_len - CIFS_MF_SYMLINK_LINK_OFFSET)
>> +               return -EINVAL;
>> +
>>          rc = symlink_hash(link_len, link_str, md5_hash);
>>          if (rc) {
>>                  cifs_dbg(FYI, "%s: MD5 hash failure: %d\n", __func__, rc);
>> --
>> 1.8.3.1
>>
> 
>