From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 12E863C9EE7 for ; Thu, 26 Mar 2026 10:40:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=193.142.43.55 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774521658; cv=none; b=HHq3mGN0O63w6waVMci3VS3yLLOtWbMTlQ6wbGWwpaQHA+UOfYT6NAeZYL4LmVMjuK4anWPycEkKpA6XZpUyxZcOq1SZ4beB3Ip9rAhA7X/EOJlmT+uo6DXan9wpZJtTDnp34bPlKcsPkxr14wkvwKhH7aQ639REsUvdjejj57I= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774521658; c=relaxed/simple; bh=au+2Blg4+MyOCS73CSApQBM9wcjrrea+60expicDpaM=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=d5ohnIVSLEqA1I0zDl2QSjOJiV+tt5QQKGqVDyYcrLPCB2WgzPqmFTKYEA4zDPhPLTmIAGZC8dbeIan9EhNL49Sgt0wsvFxgEj6wLm2xaC55E1WAhkjLgw10KXK91c/iJMJRVNEe4GxLlwo+vaWxqNRcC0NlOdMGDnmJFxWN9V8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linutronix.de; spf=pass smtp.mailfrom=linutronix.de; dkim=pass (2048-bit key) header.d=linutronix.de header.i=@linutronix.de header.b=IVFd2ls+; dkim=permerror (0-bit key) header.d=linutronix.de header.i=@linutronix.de header.b=U45wYeh5; arc=none smtp.client-ip=193.142.43.55 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linutronix.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linutronix.de Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="IVFd2ls+"; dkim=permerror (0-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="U45wYeh5" From: John Ogness DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1774521648; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=73kwmKCet9OTyVICgZnCHt9rL0OUHni1mQzysvfRP8U=; b=IVFd2ls+kMPAWIwRxmERIL9zO/MvRv+d9W4XoSSdj1j+tDVD3lxjQwtSserAttSwMXfAJA O8QzyBlAij/lkmuWrzVBHYvAbqMmfl34Uv4UwKfxnlgKOlY71279o1q3z01wcRxICVCNNC aQH+zo3HYOzNeTs99PQe9cOYcBwVhZiKrMBvt9yGLE6PYE0/Chjz57lomhJryCAJaEipYL 3QdC8vHcShOtLqNG1g6lNnKLJVg6uClpfASn8b8f+I1e5x+LEQR9gIyKvX5KuLdxvXqmec NFDTXr17ylT0nAuKwUCA5ubYiGKtk5j7039OAHZXuPjhtwEHBzNp+JPXOg7dPw== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1774521648; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=73kwmKCet9OTyVICgZnCHt9rL0OUHni1mQzysvfRP8U=; b=U45wYeh5ctaiHoT2HjzyJfvRPrnntCjd68tIf/FHt/QgnGO/+1+AGnoQ91tDcOCC+8PCLg j7aUyCDun0oViCCg== To: Petr Mladek Cc: Sergey Senozhatsky , Steven Rostedt , linux-kernel@vger.kernel.org Subject: Re: [PATCH printk] printk_ringbuffer: Fix get_data() size sanity check In-Reply-To: References: <20260319134955.185123-1-john.ogness@linutronix.de> <87fr5o570i.fsf@jogness.linutronix.de> Date: Thu, 26 Mar 2026 11:46:48 +0106 Message-ID: <87cy0qyitr.fsf@jogness.linutronix.de> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain On 2026-03-26, Petr Mladek wrote: > So, just to be sure that the new code works as expected. > Note that the moved check: > > /* Sanity check. Data-less blocks were handled earlier. */ > if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size)) > return NULL; > > warns only when data_check_size() fails. It just quietly returns NULL > when *data_size is zero. ??? The above code warns for !*data_size as well. > If we really want to warn. Then it would make more sense to change "<" > to "<=" in the previous check before the subtraction. Yes, actually I would prefer that. Let me send a v2 where I relocate and reduce the data_check_size()-WARN and extend the data_size-WARN. So it is something like this: diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c index 56c8e3d031f49..aa4b39e94cfa2 100644 --- a/kernel/printk/printk_ringbuffer.c +++ b/kernel/printk/printk_ringbuffer.c @@ -1302,23 +1302,26 @@ static const char *get_data(struct prb_data_ring *data_ring, return NULL; } - /* Sanity check. Data-less blocks were handled earlier. */ - if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size)) - return NULL; - /* A valid data block will always be aligned to the ID size. */ if (WARN_ON_ONCE(blk_lpos->begin != ALIGN(blk_lpos->begin, sizeof(db->id))) || WARN_ON_ONCE(blk_lpos->next != ALIGN(blk_lpos->next, sizeof(db->id)))) { return NULL; } - /* A valid data block will always have at least an ID. */ - if (WARN_ON_ONCE(*data_size < sizeof(db->id))) + /* + * A regular data block will always have an ID and at least + * 1 byte of data. Data-less blocks were handled earlier. + */ + if (WARN_ON_ONCE(*data_size <= sizeof(db->id))) return NULL; /* Subtract block ID space from size to reflect data size. */ *data_size -= sizeof(db->id); + /* Sanity check the max size of the regular data block. */ + if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size))) + return NULL; + return &db->data[0]; } John