linux-kernel - Re: [PATCH v1] driver:staging:vme:Remove NULL check of list

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3e6423eb-0845-4ab2-8d92-86da2c814569@stanley.mountain>
Date: Thu, 22 Aug 2024 12:52:28 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Yuesong Li <liyuesong@...o.com>
Cc: gregkh@...uxfoundation.org, soumya.negi97@...il.com,
	piroyangg@...il.com, andi.shyti@...ux.intel.com,
	alexondunkan@...il.com, linux-kernel@...r.kernel.org,
	linux-staging@...ts.linux.dev, opensource.kernel@...o.com
Subject: Re: [PATCH v1] driver:staging:vme:Remove NULL check of list_entry()

I think Greg may have already merged your commit, which I'm okay with
because so far as I can see it's fine.  But there should normally be
some additional analysis for this type of patch.

On Thu, Aug 22, 2024 at 10:57:36AM +0800, Yuesong Li wrote:
> list_entry() will never return a NULL pointer, thus remove the
> check.
> 

This is true.  But the other possibility here is that it could be that
list_entry_or_null() was intended.

In other words, sure, this patch doesn't introduce new crashing bugs, but it
might going against the work that static checker developers do to find risky
code.

The first thing I would do would be to see which commit introduced this.
git log -p --follow drivers/staging/vme_user/vme.c
This issue was introduced in 2009.  Probably if the code has been this way for
15 years and no one has complained then it's fine to remove the NULL check.

To be honest, that's probably all the analysis you need.  :P  I did a little bit
more analysis using Smatch.  These are the places where Smatch says that
->entry is set.  You'd have to build the cross function database using
~/smatch/smatch_scripts/build_kernel_data.sh and then run
`smatch/smatch_data/db/smdb.py where vme_resource entry`.

drivers/staging/vme_user/vme_user.c | vme_user_probe                 | (struct vme_resource)->entry | min-max
drivers/staging/vme_user/vme_user.c | vme_user_remove                | (struct vme_resource)->entry | min-max
drivers/staging/vme_user/vme.c | vme_slave_request              | (struct vme_resource)->entry | 0-u64max
drivers/staging/vme_user/vme.c | vme_slave_free                 | (struct vme_resource)->entry | min-max
drivers/staging/vme_user/vme.c | vme_master_request             | (struct vme_resource)->entry | 0-u64max
drivers/staging/vme_user/vme.c | vme_master_free                | (struct vme_resource)->entry | min-max
drivers/staging/vme_user/vme.c | vme_dma_request                | (struct vme_resource)->entry | 0-u64max
drivers/staging/vme_user/vme.c | vme_dma_free                   | (struct vme_resource)->entry | min-max
drivers/staging/vme_user/vme.c | vme_lm_request                 | (struct vme_resource)->entry | 0-u64max
drivers/staging/vme_user/vme.c | vme_lm_free                    | (struct vme_resource)->entry | min-max

When you look at the code, ->entry gets pointed to an entry in the list in the
request function and never modified again.

Which is slightly weird.  In other words, struct vme_resource)->entry is not
used as a list at all so far as I can see.  It's unclear to me what's going on
with vme, but I suspect we're going to remove it.  See 35ba63b8f6d0 ("vme: move
back to staging").  Otherwise the temptation would be to ask that we set a
pointer directly to slave_image and master_image instead of saving a pointer to
entry.

regards,
dan carpenter