From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f51.google.com (mail-wm1-f51.google.com [209.85.128.51]) (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 93B5D39E184 for ; Mon, 30 Mar 2026 21:49:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774907362; cv=none; b=LWirhUKVloYMzwzwBAfJ/dyWhHXWxuQb0TyQnnIPaqrZV4d2kw7IaRo8f9/Yj3YvY1F8Tb1RgyRVHoXT2CmQ4V9mPd/3rxZkJMlG8GOHBn4qwduBtCgj1bbgc/7X1KYXbDHrO8POHbUIrbZLcFA0X7AHY12FT5J8w0aB7gGefhA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774907362; c=relaxed/simple; bh=OQEN2V1zf5XX4OmThoVCUFjgNAjn/1RVXrg71cZAxBQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=h3Tk/BYoRSpjbonmQWmbobR+aChHlYxtWc1SXZA7Ln7avSnGsqCCNFrJK+TBc1f0GHF3RiMpyN+G1m7oknMJNRHcInAoOtjVHGc7DtC8GMMMmRAlMlDwHbjBZzgWaBUdef+0pmPBEHusV5wt5TWfw5BCEA5Huoq7PkescQYZCMs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=philpotter.co.uk; spf=pass smtp.mailfrom=philpotter.co.uk; dkim=pass (2048-bit key) header.d=philpotter-co-uk.20230601.gappssmtp.com header.i=@philpotter-co-uk.20230601.gappssmtp.com header.b=IBV5Asfe; arc=none smtp.client-ip=209.85.128.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=philpotter.co.uk Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=philpotter.co.uk Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=philpotter-co-uk.20230601.gappssmtp.com header.i=@philpotter-co-uk.20230601.gappssmtp.com header.b="IBV5Asfe" Received: by mail-wm1-f51.google.com with SMTP id 5b1f17b1804b1-486fda2a389so40749765e9.1 for ; Mon, 30 Mar 2026 14:49:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philpotter-co-uk.20230601.gappssmtp.com; s=20230601; t=1774907358; x=1775512158; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=iCtJw/YgoH39kldsewaUagjUei5D9Up/aGMmGXTIykc=; b=IBV5AsfeD1xJjHb3hUQ23H6HNZH+qUQ0LLqFofaEjDCzMWxMtDaRZ6EZMeJxTKI4Qq Etjg9JNhN2/JlFZW0npsvuQpSMc+9asur2KXBNDfp0Tw4s9yklcQfDg4ZTYQQwx+MCJ7 zGtAxPNp4GWx8GT3IghoUGw4qdgdjZn+8sX2ZAWOQjqdlWx7ViCdR5g2xHR9p47UF00x jabF+DQDqIKI37QnpTp2/ygbBycSEiszCYNX9nOK0iDwqaG/6br/lv4+CeW3VQqRLfO4 LvoXEVfr1KaJaX4fgirpuz8f5bs3D6SS8eNcDHI7dgBF62DEMUPDVBgY5nxqklMBK//e M97g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774907358; x=1775512158; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=iCtJw/YgoH39kldsewaUagjUei5D9Up/aGMmGXTIykc=; b=SK9FbHFFnD2m5FDTBsplHqplv3WDp55ReFg3fLA/Gedk+eWzxFTqOT8pnLuPf43GCg YIT13Y8JZyPef6/FOazQyTUYXKCkL/nSfsJaJmSl30ua0DOGVXpKOBlXc1OF1SHWCo22 LNkLelQquixT1T/O+5kwAyw/f5dbjwKGg5vWw1P0zhKnon4dHdNWarlY/K65vi+KiazL uOFw8qrfhDtOfFIJAUSm7WW6JtSUyTd3FnJWr/qrGmD01P6nu82xqGH+wvcwcMPSQwB/ KmTh4w+JcG9tYozLEngQwH2pViFuyD2Lir3MYakbKjK27P7mwPtgt80N3crVXfeJq3m7 Amww== X-Forwarded-Encrypted: i=1; AJvYcCWrLVy83bkfzImVw/0Kd+5Tn+cikviJyrpL0A+NUUUuXBRMSSOwhcHtgJ8rUGVz5SHHsgdyhs1gA2++0kY=@vger.kernel.org X-Gm-Message-State: AOJu0YwMtS0/mtcPsH/WNKxqQ41QhCVJX3+pn/Kp12Teb2t6d26e6dyP 1dLVPNqde44S376mSB5vIGqD9Bregy+6QO/I/XpVmoX2iRFUNt4bCHuFJn+woqn9hVk= X-Gm-Gg: ATEYQzxrLL3PFauAf9YwBjg49ZIIEOqp5ZXfsp1bWwxDe5zuAQPXQIrFDC6MaSj68Gm pbNvOaDRhdj6YAQv67rf7813ohZXUeZDdOpgMMUn7suZzYkaC0Th0CEU5iJSpukzM9dSVfTMTSH SnTd6DdTyY5sXQgXEwetAGG1Zw41nTKNZ1jDMXtUMVuZjBv9diDF1PB52tsO5B/fNWNwTg0aJuS bQXUTh5ZUHOfMnpJY/zFH0GvuL38hCjAE7tqpHAlnmdAzQZq9Dk7Uvp5YVoW7yzWNlgd2WvyRv7 2Axlcf70H45FZbYNZZl1pqrLJE4qd8sRzWtr2lDGHUb9WBqgbFJth7HmLfE/9YdUPyjAiQi6r8y XrfM2sXNWYXU242jN8hVXp4703DjMkspN4B62q0W2TZALVhuj/HvHSwK2RrggpSbU4QbCnj0gWt 8+1obSnvmwcQfj11ZK2E1YUalfPufT+5wHPk77BUJG5CcjZHtORoC8Zt+n7sIIr0oqIbyXHKeku AaB99stww== X-Received: by 2002:a05:600c:6092:b0:485:30d4:6b9e with SMTP id 5b1f17b1804b1-48727ef0284mr265075275e9.21.1774907357735; Mon, 30 Mar 2026 14:49:17 -0700 (PDT) Received: from equinox (2.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.6.1.f.d.0.b.8.0.1.0.0.2.ip6.arpa. [2001:8b0:df16::2]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4873ab203e9sm113117135e9.0.2026.03.30.14.49.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Mar 2026 14:49:17 -0700 (PDT) Date: Mon, 30 Mar 2026 22:49:15 +0100 From: Phillip Potter To: Felix Busch Cc: James.Bottomley@hansenpartnership.com, martin.petersen@oracle.com, phil@philpotter.co.uk, linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org Subject: Re: [PATCH 1/1] CD-ROM: Additional LBA bound check Message-ID: References: <20260325065335.7783-1-felix.busch1@gmx.net> <20260325074401.6530-1-felix.busch1@gmx.net> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260325074401.6530-1-felix.busch1@gmx.net> Hi Felix, Nice to hear from you again, hope you're well. Apologies for me taking a few days to reply, I've had a a fair amount on at work. On Wed, Mar 25, 2026 at 08:44:01AM +0100, Felix Busch wrote: > Upper bound check for the logical block address > in mmc_ioctl_cdrom_read_data() of the CD-ROM driver. > This prevents trying to read a block when the LBA is > greater than the number of available blocks. > > Signed-off-by: Felix Busch > --- > drivers/cdrom/cdrom.c | 7 +++++-- > drivers/scsi/sr.c | 12 +++++++++++- > include/linux/cdrom.h | 2 ++ > 3 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c > index fc049612d6dc..cc0a6c0ae9e7 100644 > --- a/drivers/cdrom/cdrom.c > +++ b/drivers/cdrom/cdrom.c > @@ -2926,6 +2926,8 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi, > { > struct scsi_sense_hdr sshdr; > struct cdrom_msf msf; > + const struct cdrom_device_ops *cdo = cdi->ops; > + u64 nr_blocks; > int blocksize = 0, format = 0, lba; > int ret; > > @@ -2944,8 +2946,9 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi, > if (copy_from_user(&msf, (struct cdrom_msf __user *)arg, sizeof(msf))) > return -EFAULT; > lba = msf_to_lba(msf.cdmsf_min0, msf.cdmsf_sec0, msf.cdmsf_frame0); > - /* FIXME: we need upper bound checking, too!! */ > - if (lba < 0) > + nr_blocks = cdo->get_capacity(cdi); This (as with other function pointers that are part of cdrom_device_ops, should be checked first to see if it's initialised? (it is for sr.c of course, but what about others?) > + /* Lower and upper bound check for logical block address. */ > + if ((lba < 0) || (lba > nr_blocks - 1)) > return -EINVAL; The point James makes about the return value now being different (as it would previously have been an error from the device itself) is a good one. This is probably the hardest question to answer in terms of which software may or may not break. > > cgc->buffer = kzalloc(blocksize, GFP_KERNEL); > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c > index 7adb2573f50d..a056c72341c4 100644 > --- a/drivers/scsi/sr.c > +++ b/drivers/scsi/sr.c > @@ -120,6 +120,8 @@ static int sr_packet(struct cdrom_device_info *, struct packet_command *); > static int sr_read_cdda_bpc(struct cdrom_device_info *cdi, void __user *ubuf, > u32 lba, u32 nr, u8 *last_sense); > > +static u64 sr_get_nr_blocks(struct cdrom_device_info *cdi); > + > static const struct cdrom_device_ops sr_dops = { > .open = sr_open, > .release = sr_release, > @@ -134,6 +136,7 @@ static const struct cdrom_device_ops sr_dops = { > .audio_ioctl = sr_audio_ioctl, > .generic_packet = sr_packet, > .read_cdda_bpc = sr_read_cdda_bpc, > + .get_capacity = sr_get_nr_blocks, > .capability = SR_CAPABILITIES, > }; > > @@ -142,6 +145,13 @@ static inline struct scsi_cd *scsi_cd(struct gendisk *disk) > return disk->private_data; > } > > +static inline u64 sr_get_nr_blocks(struct cdrom_device_info *cdi) > +{ > + struct scsi_cd *cd = scsi_cd(cdi->disk); > + > + return cd->capacity; > +} > + > static int sr_runtime_suspend(struct device *dev) > { > struct scsi_cd *cd = dev_get_drvdata(dev); > @@ -782,7 +792,7 @@ static int get_sectorsize(struct scsi_cd *cd) > sector_size = 2048; > fallthrough; > case 2048: > - cd->capacity *= 4; > + //cd->capacity *= 4; This is here for a reason, it should not be commented out. I may not have worded it clearly in my response to your previous patch, but this is the kind of thing I was talking about before. > fallthrough; > case 512: > break; > diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h > index b907e6c2307d..406e6f4a55bb 100644 > --- a/include/linux/cdrom.h > +++ b/include/linux/cdrom.h > @@ -91,6 +91,8 @@ struct cdrom_device_ops { > struct packet_command *); > int (*read_cdda_bpc)(struct cdrom_device_info *cdi, void __user *ubuf, > u32 lba, u32 nframes, u8 *last_sense); > + /* Get size in blocks */ > + u64 (*get_capacity)(struct cdrom_device_info *cdi); I notice we only define an implementation of this for sr.c? At a glance, it's also possible to trigger this code from the GD-ROM driver, unless I'm missing something? > /* driver specifications */ > const int capability; /* capability flags */ > }; > -- > 2.53.0 > I take your point about avoiding the read entirely, and I see what you mean there. That said, I'm not sure this is necessary given the potential dangers. Regards, Phil