| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACjpba5iJ5dC=rB_Ckaqe4BKesrAN2VmsDCPZJ=frufNgA96Uw@mail.gmail.com>
Date: Thu, 23 May 2024 23:13:08 +0800
From: shichao lai <shichaorai@...il.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: gregkh@...uxfoundation.org, oneukum@...e.com, linux-usb@...r.kernel.org,
usb-storage@...ts.one-eyed-alien.net, linux-kernel@...r.kernel.org,
xingwei lee <xrivendell7@...il.com>, yue sun <samsun1006219@...il.com>
Subject: Re: [PATCHv2] Check whether divisor is non-zero before division
On Thu, May 23, 2024 at 10:15 PM Alan Stern <stern@...land.harvard.edu> wrote:
>
> On Thu, May 23, 2024 at 09:58:21AM -0400, Alan Stern wrote:
> > On Thu, May 23, 2024 at 05:26:08PM +0800, Shichao Lai wrote:
> > > Since uzonesize may be zero, so judgements for non-zero are nessesary in both place.
> > > Previous check is moved out of loop, and one more check is added in alauda_write_lba.
> > >
> > > Reported-by: xingwei lee <xrivendell7@...il.com>
> > > Reported-by: yue sun <samsun1006219@...il.com>
> > > Signed-off-by: Shichao Lai <shichaorai@...il.com>
> > > ---
> > > drivers/usb/storage/alauda.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c
> > > index 115f05a6201a..a6e60ef5cb0d 100644
> > > --- a/drivers/usb/storage/alauda.c
> > > +++ b/drivers/usb/storage/alauda.c
> > > @@ -818,6 +818,8 @@ static int alauda_write_lba(struct us_data *us, u16 lba,
> > > unsigned int blocksize = MEDIA_INFO(us).blocksize;
> > > unsigned int lba_offset = lba % uzonesize;
> > > unsigned int new_pba_offset;
> > > + if (!uzonesize)
> > > + return USB_STOR_TRANSPORT_ERROR;
> > > unsigned int zone = lba / uzonesize;
> > >
> > > alauda_ensure_map_for_zone(us, zone);
> > > @@ -923,6 +925,8 @@ static int alauda_read_data(struct us_data *us, unsigned long address,
> > > unsigned int uzonesize = MEDIA_INFO(us).uzonesize;
> > > struct scatterlist *sg;
> > > int result;
> > > + if (!uzonesize)
> > > + return USB_STOR_TRANSPORT_ERROR;
> > >
> > > /*
> > > * Since we only read in one block at a time, we have to create
> >
> > This is definitely NOT the right way to fix the bug!
> >
> > uzonesize is set once, when the device is probed, in
> > alauda_init_media(). That is where the check belongs; if uzonesize is 0
> > then the function should print a warning and return
> > USB_STOR_TRANSPORT_ERROR, because the device is unusable.
> >
> > It's probably a good idea to check pagesize, blocksize, and zonesize at
> > the same time, even though none of them are used for any divisions.
>
> Wait a minute. I just went through the code more carefully. It should
> not be possible for uzonesize to be 0, because it is defined by:
>
> MEDIA_INFO(us).uzonesize = ((1 << media_info->zoneshift) / 128) * 125;
>
> where media_info->zoneshift is always a value between 8 and 12.
>
> So the whole idea behind this patch is misguided. The real problem is
> to find out why uzonesize ended up being 0.
>
> (And it's not necessary to check pagesize, blocksize, or zonesize,
> because none of them can ever be 0 either.)
>
> Alan Stern
Thanks for your comprehensive analysis.
I added some pr_info() to check the workflow, and I found that the
uzonesize was not initialized in fact!
The workflow is shown as below.
Before alauda_read_data(), there are in fact many alauda_check_media(),
but none of them enter the branch of alauda_init_media(), where
uzonesize is set to nonzero value.
The key branch condition is "status[0] & 0x08", which is always
unsatisfied in this repro.
```
alauda_transport
alauda_check_media
if (status[0] & 0x08) // not satisfied
alauda_init_media()
// initialize uzonesize
alauda_read_data
```
I also print status[0] before the branch, which may be helpful for you
to analyze.
The part you should focus on is the information beginning with
"alauda_check_media".
e.g. "alauda_check_media: before alauda_get_media_status, status[0]:
0000000000000000" means in alauda_check_media(), before calling
alauda_get_media_status()
It seems that alauda_get_media_status() will transform the status[0]
to 0x0000000000000036, which doesn't satisfy the condition of
"status[0] & 0x08".
===========
root@...kaller:~# ./exp
[ 28.645451][ T2386] usb 1-1: new high-speed USB device number 2
using dummy_hcd
[ 28.885289][ T2386] usb 1-1: Using ep0 maxpacket: 16
[ 29.005519][ T2386] usb 1-1: config 0 has an invalid interface
number: 192 but max is 0
[ 29.007448][ T2386] usb 1-1: config 0 has no interface number 0
[ 29.008759][ T2386] usb 1-1: config 0 interface 192 altsetting 0
endpoint 0x9 has invalid wMaxPacketSize 0
[ 29.010799][ T2386] usb 1-1: config 0 interface 192 altsetting 0
bulk endpoint 0x9 has invalid maxpacket 0
[ 29.012467][ T2386] usb 1-1: config 0 interface 192 altsetting 0
endpoint 0x8F has invalid maxpacket 59960, setting to 1024
[ 29.012959][ T2386] usb 1-1: config 0 interface 192 altsetting 0
bulk endpoint 0x8F has invalid maxpacket 1024
[ 29.013413][ T2386] usb 1-1: New USB device found, idVendor=07b4,
idProduct=010a, bcdDevice= 1.02
[ 29.013809][ T2386] usb 1-1: New USB device strings: Mfr=0,
Product=0, SerialNumber=0
[ 29.015193][ T2386] usb 1-1: config 0 descriptor??
[ 29.035791][ T4124] raw-gadget.0 gadget.0: fail, usb_ep_enable returned -22
[ 29.057576][ T2386] ums-alauda 1-1:0.192: USB Mass Storage device detected
[ 29.062276][ T2386] scsi host2: usb-storage 1-1:0.192
[ 30.098931][ T2386] scsi 2:0:0:0: Direct-Access Olympus
MAUSB-10 (Alauda 0102 PQ: 0 ANSI: 0 CCS
[ 30.102903][ T4131] alauda_check_media: before
alauda_get_media_status, status[0]: 0000000000000000
[ 30.104297][ T2386] sd 2:0:0:0: Attached scsi generic sg2 type 0
[ 30.135805][ T4131] alauda_get_media_status: data=54, rc=0
[ 30.137113][ T4131] alauda_check_media: after
alauda_get_media_status, status[0]: 0000000000000036
[ 30.138991][ T4131] alauda_check_media: before init_media,
status[0]: 0000000000000036
[ 30.141727][ T4131] alauda_check_media: before
alauda_get_media_status, status[0]: 0000000000000036
[ 30.355544][ T4131] alauda_get_media_status: data=54, rc=0
[ 30.356815][ T4131] alauda_check_media: after
alauda_get_media_status, status[0]: 0000000000000036
[ 30.358728][ T4131] alauda_check_media: before init_media,
status[0]: 0000000000000036
[ 30.361408][ T4131] alauda_check_media: before
alauda_get_media_status, status[0]: 00000000000000ff
[ 30.575607][ T4131] alauda_get_media_status: data=54, rc=0
[ 30.576910][ T4131] alauda_check_media: after
alauda_get_media_status, status[0]: 0000000000000036
[ 30.578823][ T4131] alauda_check_media: before init_media,
status[0]: 0000000000000036
[ 30.580999][ T4131] alauda_check_media: before
alauda_get_media_status, status[0]: 00000000000000ff
[ 30.795625][ T4131] alauda_get_media_status: data=54, rc=0
[ 30.796899][ T4131] alauda_check_media: after
alauda_get_media_status, status[0]: 0000000000000036
[ 30.798773][ T4131] alauda_check_media: before init_media,
status[0]: 0000000000000036
[ 30.801017][ T4131] alauda_check_media: before
alauda_get_media_status, status[0]: 00000000000000ff
[ 31.015362][ T4131] alauda_get_media_status: data=54, rc=0
[ 31.016679][ T4131] alauda_check_media: after
alauda_get_media_status, status[0]: 0000000000000036
[ 31.018565][ T4131] alauda_check_media: before init_media,
status[0]: 0000000000000036
[ 31.020543][ T263] sd 2:0:0:0: [sdb] Very big device. Trying to
use READ CAPACITY(16).
[ 31.023323][ T263] sd 2:0:0:0: [sdb] Using 0xffffffff as device size
[ 31.035382][ T2386] scsi 2:0:0:1: Direct-Access Olympus
MAUSB-10 (Alauda 0102 PQ: 0 ANSI: 0 CCS
[ 31.035466][ T4131] alauda_transport: before alauda_check_media
[ 31.038810][ T4131] alauda_check_media: before
alauda_get_media_status, status[0]: 0000000000000000
[ 31.043663][ T2386] sd 2:0:0:1: Attached scsi generic sg3 type 0
[ 31.235486][ T4131] alauda_get_media_status: data=54, rc=0
[ 31.237015][ T4131] alauda_check_media: after
alauda_get_media_status, status[0]: 0000000000000036
[ 31.239266][ T4131] alauda_check_media: before init_media,
status[0]: 0000000000000036
[ 31.241273][ T4131] alauda_transport: after alauda_check_media ->
alauda_read_data
[ 31.243134][ T4131] alauda_read_data: 0
[ 31.244148][ T4131] divide error: 0000 [#1] PREEMPT SMP KASAN NOPTI
Powered by blists - more mailing lists