From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f53.google.com (mail-wr1-f53.google.com [209.85.221.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BBE5B1F63ED for ; Fri, 17 Jan 2025 10:32:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.53 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737109942; cv=none; b=jCCMMFLz16voWI3CfOqYe3XrkemepmR8pBzyIvf/EWJxYg13YugYaEea014HtrhGq0RBvl3D2ager6nlQwjafueSDKDbvu6YxBRMiSU2Yq7bSgf1VzVH1CBlCvW/EBw6tWCYZKsntujRuCBF0YM8sjgIiLmAtH5ZK39QyMFX/BY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737109942; c=relaxed/simple; bh=usxKX/bq+qqlj5S9Kv1IxI4efxTMwXfcJQVp97tlIL8=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=LrGqIJ+4UWoh4cptm57WS+JOMR+sYXAgejAxx67q/HRpd8bFXmB9nNjkaDR85GDAmHVBlk96K/2oAvzuuAYPINNEZfQ47TYs3phAHJ2kA+g8w9kNOJbCL7I5Bn8pV+cvc//y/91XNfNY0x3PRuH5zWBN5kn2XyyEIVlN9iNEFF8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=SPWNYRnG; arc=none smtp.client-ip=209.85.221.53 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="SPWNYRnG" Received: by mail-wr1-f53.google.com with SMTP id ffacd0b85a97d-3862d161947so1043487f8f.3 for ; Fri, 17 Jan 2025 02:32:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1737109939; x=1737714739; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=HksGXKs+oXl9+VmDEVOiawBWVGC/PHhWQk+TriN1KVo=; b=SPWNYRnGeR83yO40FNqkDIDHQANAeZhA3YdcNJcx2Xx97rpzs53+OUqiRaFwckdOUC P/RnqDTNt+raumEqSs4swSlntOQiBEQXPvN+nI0lNtyQTLSeuIDzZFOpPieqnm90nzLS WyfQWOT6I7rzPjVKpwiCITIUiyNHvebVCRM8rrSghF9Yk1VqWaWeWQ4eLwq/nIWMP+kf iEshZa7xBzDVPAHHdCI1ce2nxeqG3Oc1pBGtgAJQnvDQ3RPE9B1XJfpxa1Y0jI6xFCU7 EdWtuFnQnCeT6DcBj6kG00BU7Aq3F4fncOK/2HZ92MM9E5FvFKx22DIDKsqHHqHvk7/D t4Zw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737109939; x=1737714739; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=HksGXKs+oXl9+VmDEVOiawBWVGC/PHhWQk+TriN1KVo=; b=qKjCNz8kWDySrsuao84ZzIzF5Kp0hQnHUS6mW1PLDot9W455bsS1QGXyWEURHrlYAa PolhuuUJjHOzwwoh7qPu5YhtnGuQddrjtMaVB4xB1YRV6I1umqmQvCJmoibis511uzme ogVd3i8oB3odHGiV+JMca0xKM5KkOwJgfne68dkuTnIzwUCBpL1EXdXNAtzmzoN5COa0 52PCZyWHHMutZgtzIBrxbA8JjwjBAl//OeV+gfZonLfu6IWbQaDDrsPdtnt168s2v3r7 xs+VAX8NJRijhrXv9BEqA6z4rcuuob1q7uLJiBCLiymHsnztwkYHBLHjm7l3NP7YxrZX 0w9w== X-Forwarded-Encrypted: i=1; AJvYcCXSW9jFfG3d/hoblLaszDq9GNTSsZkaxy7TmVsoemWQkqGDTiO+xqV4FLOKt13MYFuMQxoTY3tbJyCEnGyA@vger.kernel.org X-Gm-Message-State: AOJu0Yzc1hyLMCACXIVHVk95qvFvwZ4+TkpmFR8NN7SrzhzpsjIo3SJi SAHa3qkhvEFFMXRdRSve/8z4vhS0Zilq48zwQ7y6ML5AngrJKgDgDtC0evTnkZg= X-Gm-Gg: ASbGncsd0Uw+nzlzEfhoUFH1jncJ3tqdg5O0ApLDWokqZGgr7dYbe4MWTmWygRrnunQ a4t782pNfC2T289U1YmhOYF+E/3ER/WzPULgxTUKvlQjaBXpsl/r9vBKuXteaxBYTeWJ8TckUBA xsnj4Yb+AWjsUIn2yMybZDTTBViTr1z3rI46vA7le0ddZSQcH6CctfvvebuXlkKH1I0VhsN94Fr aHs/1sF/WoJ0WD8YvvPxiVrvrEoOZ5qaQw5hHMNixzuZn5vdGSj1n0J/Dnbr3LIXwIaJQ== X-Google-Smtp-Source: AGHT+IG7Rquzi6WwJgROhsE/JHaxo92uKlPy8ctz5KPUZeEVLYQDkbB3Dqa5Mk08B3VGfm3jM2JWmg== X-Received: by 2002:a5d:4845:0:b0:385:f573:1f78 with SMTP id ffacd0b85a97d-38bf566e2b2mr1467481f8f.24.1737109939033; Fri, 17 Jan 2025 02:32:19 -0800 (PST) Received: from [192.168.0.35] ([176.61.106.227]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38bf3275562sm2191384f8f.66.2025.01.17.02.32.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 17 Jan 2025 02:32:18 -0800 (PST) Message-ID: <0eab7323-ce86-40c7-9737-06eedcdf492d@linaro.org> Date: Fri, 17 Jan 2025 10:32:17 +0000 Precedence: bulk X-Mailing-List: linux-arm-msm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] media: venus: fix OOB access issue while reading sequence changed events To: Vedang Nagar , Stanimir Varbanov , Vikash Garodia , Mauro Carvalho Chehab Cc: linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org References: <20250104-venus-security-fixes-v1-0-9d0dd4594cb4@quicinc.com> <20250104-venus-security-fixes-v1-2-9d0dd4594cb4@quicinc.com> <2b0528f5-f9fa-4cfd-abda-a0e95ba4a2f1@linaro.org> <7a782ea9-f227-4f46-a757-b4b69f5c287f@quicinc.com> Content-Language: en-US From: Bryan O'Donoghue In-Reply-To: <7a782ea9-f227-4f46-a757-b4b69f5c287f@quicinc.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 17/01/2025 08:39, Vedang Nagar wrote: > Hi Bryan, > > On 1/6/2025 5:36 AM, Bryan O'Donoghue wrote: >> On 04/01/2025 05:41, Vedang Nagar wrote: >>> num_properties_changed is being read from the message queue but is >>> not validated. Value can be corrupted from the firmware leading to >>> OOB read access issues. Add fix to read the size of the packets as >>> well and crosscheck before reading from the packet. >>> >>> Signed-off-by: Vedang Nagar >> Please see Vikash's series on this. >> >> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-2-483ae0a464b8@quicinc.com/ >> >> it seems to have exactly the same patch title ? >> >> Is this patch supposed to be a follow-up to that patch ? >> >> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-0-483ae0a464b8@quicinc.com/ >> >> Expecting to see a V3 of the above. If the intention is to supersede that patch or some of those patches you should make clear here. > No, this is a different series having OOB fixes similar to ones posted by Vikash. OK, please use a different patch title. I understand the motive to repeat the patch title but, its confusing. If you added some text to make the OOB more specific then it would be possible to differentiate between. "fix OOB access issue while reading sequence changed events 'in some location' || 'on some path'" >> >> On the switch statement I'd have two comments. >> >> #1 is everything really a " -= sizeof(u32)" ? > Yes, it's everytime " -= sizeof(u32) " since the first the first word read is ptype of size u32 >> #2 if so then this ought to be factored out into a function >>    => functional decomposition > Sure, will fix this with decomposition into functions. Is firmware sending a change event or updating a packet already in memory ? What is the nature of the change event and how do you guarantee the second read is valid when the first read can be considered invalid ? i.e. - Read - derive read value X. - Do some stuff. - Read - again to make sure length value is still X. - Do all sorts of other processing. At which point is the sequence considered complete and the data considered "locked" and valid ? What happens if you get a subsequent change event once this procedure has completed ? --- bod